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

feat(load): add cosmiconfig typescript loader #2698

Merged
merged 7 commits into from
Aug 12, 2021
Merged

feat(load): add cosmiconfig typescript loader #2698

merged 7 commits into from
Aug 12, 2021

Conversation

songhn233
Copy link
Contributor

Description

Add ts format config support.

Reference to cosmiconfig loader document, add @endemolshinegroup/cosmiconfig-typescript-loader to load the ts config according to a higher priority than the js config.

Motivation and Context

#2652

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.

@songhn233
Copy link
Contributor Author

@escapedcat request review

@escapedcat
Copy link
Member

Thank you @songhn233 for this PR and @polkovnikov-ph for the review and nitpicking ;)
You think it would be good to add a test for a ts-config-file, like we do here?:
https://github.com/conventional-changelog/commitlint/blob/master/%40commitlint/load/src/load.test.ts#L36

Let's wait for the others to review.

@songhn233
Copy link
Contributor Author

You think it would be good to add a test for a ts-config-file, like we do here?

The tests in load only include js-config-file, if ts-config-file tests are added then yaml and other format tests I think can be added as well.

@reverofevil
Copy link

reverofevil commented Jul 31, 2021

@escapedcat I tried to follow Conventional Comments. Seems to be very applicable for PRs in this repo :)

(Of course I forgot to put a (non-blocking) decorator :/)

Copy link
Member

@AdeAttwood AdeAttwood left a comment

Choose a reason for hiding this comment

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

All looks good. I am with @escapedcat and can we get a test added for it.

Thanks for getting this one in 👍

@commitlint/load/src/utils/load-config.ts Outdated Show resolved Hide resolved
Co-authored-by: Ade Attwood <code@adeattwood.co.uk>
@songhn233
Copy link
Contributor Author

@escapedcat @AdeAttwood
I added test cases, but since the fixtures mechanism makes it impossible to import types from other modules, I creat a single type.ts.

To avoid complex definitions for some properties I defined directly as any.

@jcayzac
Copy link

jcayzac commented Aug 10, 2021

The PR also fixes #2711

Copy link
Member

@AdeAttwood AdeAttwood left a comment

Choose a reason for hiding this comment

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

I think this is all good. TBH, I am not too worried about the types in this test. This test is fine to test that you can load a .ts file in the config. We can sort out other tests for the types.

@AdeAttwood AdeAttwood merged commit 885bd8a into conventional-changelog:master Aug 12, 2021
@escapedcat
Copy link
Member

Do we need to update the docs for this?
Just saw this: #83 (comment)

@escapedcat
Copy link
Member

It's released, please have a look. Thanks

@escapedcat
Copy link
Member

escapedcat commented Jul 1, 2022

Hey people, please have a look at this update PR: #3253
Happy for any feedback!

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

5 participants