-
Notifications
You must be signed in to change notification settings - Fork 7
Convert this library to ES Modules #244
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
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #244 +/- ##
==========================================
- Coverage 97.48% 97.43% -0.05%
==========================================
Files 11 11
Lines 437 429 -8
Branches 97 96 -1
==========================================
- Hits 426 418 -8
Misses 11 11
... and 1 file with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just one questions related to the values sets in tsconfig.json, compared to what we have been setting in mozilla/sign-addon#1077.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, I was wondering: should we make sure we converted the other packages that depends from this one (e.g. addons-linter is one for sure, but I recall there was at least one or two more) before merging this and releasing a version of this package that is ESM-only?
If the way this is used in the other packages can be also just use the dynamic import, we may release the ESM-version of this package independently from the order we will do the same on the other ones, but I wanted to ask you explicitly in case we'll need to merge this and release a new version on npm after we have done the same kind of conversion on the dependents.
As discussed on Slack, I am going to wait a bit and therefore mark this PR as a draft. I think ESM does not necessarily work well with pure TypeScript projects at the moment and this lib is used in one of them... I'll look into that next week or so. |
@willdurand with TypeScript 5 out, is is time to revisit this ? |
Yeah, I think the problem came from customs back then. If TS5 better supports ESM, we should be good. |
46faad9
to
e048960
Compare
We started to convert our libraries to ES Modules. This PR does that for
this package, which is written with TypeScript and obviously ESM isn't a
first class citizen there (yet?).
This is needed for converting addons-linter to ES Modules.Actually,it is more of a nice to have but not required. That being said, converting
this package will allow us to keep up with the dependency updates.