Skip to content

feat(load): add cosmiconfig typescript loader#2698

Merged
AdeAttwood merged 7 commits intoconventional-changelog:masterfrom
songhn233:master
Aug 12, 2021
Merged

feat(load): add cosmiconfig typescript loader#2698
AdeAttwood merged 7 commits intoconventional-changelog:masterfrom
songhn233:master

Conversation

@songhn233
Copy link
Copy Markdown
Contributor

@songhn233 songhn233 commented Jul 30, 2021

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.

reverofevil
reverofevil reviewed Jul 30, 2021
View reviewed changes
Comment thread @commitlint/types/src/prompt.ts Outdated
reverofevil
reverofevil reviewed Jul 30, 2021
View reviewed changes
Comment thread @commitlint/load/src/utils/load-config.ts Outdated
@songhn233
Copy link
Copy Markdown
Contributor Author

songhn233 commented Jul 31, 2021

@escapedcat request review

@escapedcat
Copy link
Copy Markdown
Member

escapedcat commented Jul 31, 2021

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

songhn233 commented Jul 31, 2021

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

reverofevil commented Jul 31, 2021 •
edited
Loading

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

AdeAttwood
AdeAttwood requested changes Aug 3, 2021
View reviewed changes
Copy link
Copy Markdown
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 👍

Comment thread @commitlint/load/src/utils/load-config.ts Outdated
@songhn233 songhn233 requested a review from AdeAttwood August 4, 2021 06:28
@songhn233
Copy link
Copy Markdown
Contributor Author

songhn233 commented Aug 5, 2021

@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.

reverofevil
reverofevil reviewed Aug 5, 2021
View reviewed changes
Comment thread @commitlint/load/fixtures/recursive-extends-ts/first-extended/index.ts
@AdeAttwood AdeAttwood mentioned this pull request Aug 9, 2021
Closed
@jcayzac
Copy link
Copy Markdown

jcayzac commented Aug 10, 2021

The PR also fixes #2711

jcayzac
jcayzac reviewed Aug 10, 2021
View reviewed changes
Comment thread @commitlint/load/src/utils/load-config.ts
AdeAttwood
AdeAttwood approved these changes Aug 11, 2021
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Member

escapedcat commented Aug 20, 2021

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

@escapedcat escapedcat mentioned this pull request Aug 20, 2021
Merged
@AdeAttwood AdeAttwood mentioned this pull request Aug 27, 2021
Closed
@escapedcat
Copy link
Copy Markdown
Member

escapedcat commented Sep 28, 2021

It's released, please have a look. Thanks

@cherryblossom000 cherryblossom000 mentioned this pull request Sep 30, 2021
Closed
1 task
@escapedcat escapedcat mentioned this pull request Jun 11, 2022
Closed
4 tasks
@escapedcat
Copy link
Copy Markdown
Member

escapedcat commented Jul 1, 2022 •
edited
Loading

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

Reviewers

@AdeAttwood AdeAttwood AdeAttwood approved these changes

@byCedric byCedric Awaiting requested review from byCedric

+2 more reviewers

@jcayzac jcayzac jcayzac left review comments

@reverofevil reverofevil reverofevil 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.

5 participants

@songhn233 @escapedcat @reverofevil @jcayzac @AdeAttwood