-
-
Notifications
You must be signed in to change notification settings - Fork 603
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
chore: bump interpret for .mjs support #1728
Conversation
/cc @evilebottnawi |
Fail 😕 |
Need update terser because a warning is coming from it which is failing the tests |
WIP on terser |
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.
Can we add tests?
Yes, running into some issue with mjs files, will update once resolved |
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.
Could you rebase?
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.
needs rebase
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.
Needs test & rebase
@anshumanv high priority before release, thanks for helping |
yes today 👍 |
Nice |
Running into little problem while adding tests for mjs config, I'm using rechoir to setup the environment and it uses require syntax to read any config format, even tried adding a package.json file with type
|
@anshumanv we should use |
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.
Could you push the testcase?
b6d68b8
to
bfb3a1a
Compare
@anshumanv Thanks for your update. I labeled the Pull Request so reviewers will review it again. @evenstensberg Please review the new changes. |
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.
We use require
to load config, right?
Alternative solutions https://github.com/standard-things/esm |
Update |
Closing in favour of #1786 |
What kind of change does this PR introduce?
chore
Did you add tests for your changes?
Yes
If relevant, did you update the documentation?
NA
Summary
Interpret has added .mjs support in latest version so bumping it then will be working on mjs support + tests
Does this PR introduce a breaking change?
No
Other information