Skip to content

refactor: port load to typescript#786

Merged
marionebl merged 19 commits intomasterfrom
refactor/load-to-ts
Feb 3, 2020
Merged

refactor: port load to typescript#786
marionebl merged 19 commits intomasterfrom
refactor/load-to-ts

Conversation

@byCedric
Copy link
Copy Markdown
Member

@byCedric byCedric commented Sep 12, 2019

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
Closed
18 tasks
@byCedric byCedric changed the title Refactor/load to ts refactor: port load to typescript Sep 12, 2019
marionebl
marionebl reviewed Sep 21, 2019
View reviewed changes
Comment thread @commitlint/cli/src/cli.js Outdated

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
Copy Markdown
Contributor

@marionebl marionebl Sep 21, 2019

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
Copy Markdown
Member Author

@byCedric byCedric Sep 21, 2019

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
Copy Markdown
Contributor

@marionebl marionebl Sep 22, 2019

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
Copy Markdown
Member Author

@byCedric byCedric Jan 26, 2020

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.

marionebl
marionebl reviewed Sep 21, 2019
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Member Author

byCedric commented Sep 21, 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.

@marionebl
Copy link
Copy Markdown
Contributor

marionebl commented Sep 22, 2019 •
edited
Loading

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
Copy Markdown
Contributor

marionebl commented Sep 22, 2019

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

@byCedric
Copy link
Copy Markdown
Member Author

byCedric commented Sep 23, 2019

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

@byCedric
Copy link
Copy Markdown
Member Author

byCedric commented Jan 26, 2020 •
edited
Loading

@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
Copy Markdown
Contributor

marionebl commented Jan 27, 2020

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 :)

@marionebl marionebl mentioned this pull request Jan 27, 2020
Closed
7 tasks
armano2
armano2 reviewed Feb 2, 2020
View reviewed changes
Comment thread 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
Copy Markdown
Contributor

@armano2 armano2 Feb 2, 2020 •
edited
Loading

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
This was referenced Feb 3, 2020
Merged
Merged
@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

Reviewers

@escapedcat escapedcat Awaiting requested review from escapedcat

2 more reviewers

@armano2 armano2 armano2 left review comments

@marionebl marionebl marionebl left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@byCedric @marionebl @armano2