Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: port load to typescript #786

Merged
merged 19 commits into from Feb 3, 2020
Merged

refactor: port load to typescript #786

merged 19 commits into from Feb 3, 2020

Conversation

byCedric
Copy link
Member

Description

Part of #659

Motivation and Context

It's kind of an old attempt from me, with some new mocking mechanisms. Instead of using proxyquire, it uses jest. With jest, we can actually import typescript, with proxyquire we cant... I also "made it work" in the CLI, by unpacking from default. Think there might be some things in here that we should change, like the any castings. Let me know what you think! 😁

Usage examples

How Has This Been Tested?

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@marionebl marionebl mentioned this pull request Sep 12, 2019
18 tasks
@byCedric byCedric changed the title Refactor/load to ts refactor: port load to typescript Sep 12, 2019
@@ -1,7 +1,8 @@
#!/usr/bin/env node
require('babel-polyfill'); // eslint-disable-line import/no-unassigned-import

const load = require('@commitlint/load');
// fix: commitlint load is ported to typescript, until this one is ported we need to unpack it
const {default: load} = require('@commitlint/load');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This amounts to a breaking change for CommonJS consumers. Can we emit the module to contain the module.exports = ... stanza instead? If i remember correctly there was a tsc settings for this...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll check it out!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't fix the breakage for external CommonJS consumers but we can use the default import interop babel provides.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should create a building library that works like bob. With such a tool we can build commonjs, module and just typescript definition files. I think that might be best to keep support for all platforms, right? Internally, we can just use the latest default module system.

Copy link
Contributor

@marionebl marionebl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good at first glance! I'll fiddle around a bit with the any types and check if we can avoid the CommonJS breakage

@byCedric
Copy link
Member Author

Yeah, I agree with the any types. My main concern was the mocking part, so I focussed on that one. About the commonjs thing, do you want to have that globally? I can imagine this being an issue in more packages, and if we centralize it we can easily switch if we want to in a later major version.

@marionebl
Copy link
Contributor

marionebl commented Sep 22, 2019

Yeah, I agree with the any types. My main concern was the mocking part, so I focussed on that one. About the commonjs thing, do you want to have that globally? I can imagine this being an issue in more packages, and if we centralize it we can easily switch if we want to in a later major version.

Good job on switching to jest mocking! Thinking about it we might speed up @commitlint/load unit tests by mocking cosmiconfig and moving the config fixtures inline. We can defer this though.

Regarding the TS setting to emit CommonJS module.exports - no luck in my research so far.

@marionebl
Copy link
Contributor

No idea why the pluginloader test acts up now...

@byCedric
Copy link
Member Author

I'll take a look tomorrow! I bet it's still repairable 😬

@byCedric
Copy link
Member Author

byCedric commented Jan 26, 2020

@marionebl I may need some help finishing this one 😄 The types you added are great, but they also overlap with types from rules (#785). Maybe we can either update them to support this set, or update this to reuse them?

I also implemented the loadParserOpts, the rewrite for the new conventional changelog parsers. I'll merge master into this one with those changes.

@marionebl
Copy link
Contributor

Maybe we can either update them to support this set, or update this to reuse them?

Yes, let's export and use the @commitlint/rules types where possible :)

Co-authored-by: Armano <armano2@users.noreply.github.com>
jest.config.js Outdated
'**/@commitlint/travis-cli/src/*.test.js?(x)',
'**/@commitlint/cli/src/*.test.js?(x)',
'**/@commitlint/prompt-cli/*.test.js?(x)'
'**/@commitlint/{lint,read,travis-cli,cli,prompt-cli,load}/src/*.test.js?(x)'
Copy link
Contributor

@armano2 armano2 Feb 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not correct as prompt-cli has no src folder

@marionebl marionebl merged commit ac71331 into master Feb 3, 2020
@escapedcat escapedcat deleted the refactor/load-to-ts branch August 1, 2021 04:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants