Skip to content

feat: add mjs support #2172

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

Merged
merged 13 commits into from
Dec 7, 2020
Merged

feat: add mjs support #2172

merged 13 commits into from
Dec 7, 2020

Conversation

rishabh3112
Copy link
Member

What kind of change does this PR introduce?
Feature

Did you add tests for your changes?
Yup
If relevant, did you update the documentation?
No
Summary
New PR that supersedes #1786

Does this PR introduce a breaking change?
No

Other information
Fresh start

@rishabh3112 rishabh3112 requested a review from a team as a code owner December 3, 2020 12:47
@rishabh3112 rishabh3112 closed this Dec 3, 2020
@rishabh3112 rishabh3112 reopened this Dec 3, 2020
Copy link
Member

@anshumanv anshumanv left a comment

Choose a reason for hiding this comment

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

mjs test failing

@alexander-akait
Copy link
Member

Yep, WIP on this

@alexander-akait
Copy link
Member

Found the problem - v8-compile-cache, doesn't support dynamic import, I will add DISABLE_V8_COMPILE_CACHE env for tests and put this in the issue as temporary workaround

@rishabh3112
Copy link
Member Author

Confirm this. Will update the PR accordingly as native import works too without v8-compile-cache.

@alexander-akait
Copy link
Member

@rishabh3112 don't touch this PR, I am working on it

@rishabh3112
Copy link
Member Author

@rishabh3112 don't touch this PR, I am working on it

I am already done with native import + added env variable to github actions locally 😞
If you like I can push that.

@alexander-akait
Copy link
Member

I am already done with native import + added env variable to github actions locally disappointed

What do you mean?

@alexander-akait
Copy link
Member

Tests should be fine now, I don't know how to better fix bug in v8-compile-cache, maybe we should move it inside webpack-cli and fix bug

@rishabh3112
Copy link
Member Author

rishabh3112 commented Dec 3, 2020

What do you mean?

I was done with required changes at that point of time with a different approach. Yours is better so never mind 🙂

Tests should be fine now, I don't know how to better fix bug in v8-compile-cache, maybe we should move it inside webpack-cli and fix bug

Yup seems way to go.

@codecov
Copy link

codecov bot commented Dec 4, 2020

Codecov Report

Merging #2172 (712fa90) into master (eda0663) will decrease coverage by 0.08%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2172      +/-   ##
==========================================
- Coverage   68.35%   68.27%   -0.09%     
==========================================
  Files          72       72              
  Lines        2345     2358      +13     
  Branches      518      520       +2     
==========================================
+ Hits         1603     1610       +7     
- Misses        742      748       +6     
Impacted Files Coverage Δ
packages/webpack-cli/lib/webpack-cli.js 94.18% <66.66%> (-2.02%) ⬇️
packages/webpack-cli/lib/utils/cli-flags.js 100.00% <0.00%> (ø)
packages/webpack-cli/lib/groups/runHelp.js 98.66% <0.00%> (+0.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eda0663...712fa90. Read the comment docs.

@alexander-akait alexander-akait force-pushed the feat/mjs-config-support branch from 2db2eeb to 4125080 Compare December 5, 2020 15:11
@alexander-akait
Copy link
Member

Very weird, it works locally, but throw an error on CI, even for node@14, tests in webpack-cli is pain

@rishabh3112
Copy link
Member Author

rishabh3112 commented Dec 5, 2020

Seems to be passing everywhere (few are still running) now. Will look into it if this run fails.

@alexander-akait
Copy link
Member

Bug on nyc side, don't have ideas how we can fix it now, let's fix it late, I will put it in TODO, maybe we need babel for nyc

@alexander-akait
Copy link
Member

Anyway es configuration is not supported fully due v8-compile-cache bug, I think I will resolve it for the next release

@alexander-akait alexander-akait merged commit 3f31702 into master Dec 7, 2020
@alexander-akait alexander-akait deleted the feat/mjs-config-support branch December 7, 2020 13:42
@anshumanv anshumanv mentioned this pull request Dec 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants