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

refactor(babel-preset-app): simplify babel preset config #6087

Merged
merged 13 commits into from Sep 3, 2019

Conversation

@clarkdo
Copy link
Member

clarkdo commented Jul 18, 2019

This is a breaking change for manually babel config user.

Changes:

  • remove buildTarget and modern, the value if from babel api now. https://babeljs.io/docs/en/config-files#apienv which is bound to webpack build (client, server, modern)
  • change param of build.babel.presets from isServer to envName which supports client, server, modern
@manniL

This comment has been minimized.

Copy link
Member

manniL commented Jul 18, 2019

if it's a breaking change, should we move it to Nuxt 3 then?

@clarkdo

This comment has been minimized.

Copy link
Member Author

clarkdo commented Jul 18, 2019

@manniL That's why this is a draft pr 😄

@manniL manniL added the 3.x label Jul 21, 2019
@clarkdo

This comment has been minimized.

Copy link
Member Author

clarkdo commented Jul 23, 2019

I can make this pr be compatible with 2.x, will change it later

@clarkdo clarkdo self-assigned this Jul 27, 2019
@clarkdo clarkdo removed the 3.x label Aug 3, 2019
@clarkdo clarkdo marked this pull request as ready for review Aug 3, 2019
@clarkdo clarkdo requested a review from nuxt/core-team Aug 3, 2019
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Aug 3, 2019

Codecov Report

Merging #6087 into dev will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##              dev    #6087   +/-   ##
=======================================
  Coverage   95.71%   95.71%           
=======================================
  Files          79       79           
  Lines        2662     2662           
  Branches      685      682    -3     
=======================================
  Hits         2548     2548           
  Misses         98       98           
  Partials       16       16
Flag Coverage Δ
#e2e 100% <ø> (ø) ⬆️
#fixtures 50.67% <100%> (ø) ⬆️
#unit 92.37% <83.33%> (ø) ⬆️
Impacted Files Coverage Δ
packages/babel-preset-app/src/index.js 96.66% <100%> (-0.11%) ⬇️
packages/webpack/src/config/base.js 94.94% <100%> (+0.05%) ⬆️

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 f1edd82...d44a14c. Read the comment docs.

@pi0

This comment has been minimized.

Copy link
Member

pi0 commented Aug 5, 2019

@clarkdo please merge if you wish :)

@clarkdo

This comment has been minimized.

Copy link
Member Author

clarkdo commented Aug 5, 2019

@pi0 Thanks, I’ll do more tests

@Atinux Atinux added this to In progress in Nuxt v2.9 Aug 6, 2019
@pi0 pi0 removed this from In progress in Nuxt v2.9 Aug 10, 2019
@Atinux Atinux added this to To do in Nuxt v2.10 Aug 21, 2019
@pi0 pi0 moved this from To do to Ready to Review in Nuxt v2.10 Aug 21, 2019
@stale

This comment was marked as off-topic.

Copy link

stale bot commented Sep 2, 2019

Thanks for your contribution to Nuxt.js!
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
If you would like this issue to remain open:

  1. Verify that you can still reproduce the issue in the latest version of nuxt-edge
  2. Comment the steps to reproduce it

Issues that are labeled as pending will not be automatically marked as stale.

@stale stale bot added the stale label Sep 2, 2019
@clarkdo clarkdo added pending and removed stale labels Sep 2, 2019
@pi0 pi0 removed the pending label Sep 3, 2019
packages/babel-preset-app/src/index.js Outdated Show resolved Hide resolved
return [
[
require.resolve('@nuxt/babel-preset-app'),
'@nuxt/babel-preset-app',

This comment has been minimized.

Copy link
@pi0

pi0 Sep 3, 2019

Member

Not sure why it was required as before. I hope it is not breaking NPX.

This comment has been minimized.

Copy link
@clarkdo

clarkdo Sep 3, 2019

Author Member

The difference is the resolving happens in node_modules/@babel/.. instead of nuxt.config.js, I think they should have same resolving result in NPM, how do you think ?

This comment has been minimized.

Copy link
@pi0

pi0 Sep 3, 2019

Member

Me too. As long as explicit dependencies are there (and webpack has explicit dependenc on babel-preset-app) so at least seems to be safe :)


if (typeof options.presets === 'function') {
options.presets = options.presets({ isServer: this.isServer }, defaultPreset)
// TODO: remove isServer in Nuxt 3

This comment has been minimized.

Copy link
@pi0

pi0 Sep 3, 2019

Member

Some may prefer this convention (as it is also in other ideas) so maybe worth having isServer/isClient/isModern with nuxt 3 or deprecate them everywhere?

This comment has been minimized.

Copy link
@clarkdo

clarkdo Sep 3, 2019

Author Member

My thought was that isServer in current babel is for user to differ babel config in different builds(server/client), with this pr, user doesn't need to check it in most cases since preset-app has done it automatically via envName.

You comment also makes sense, we can add isClient, isModern, but do we have envName as arg as well ?

This comment has been minimized.

Copy link
@pi0

pi0 Sep 3, 2019

Member

Don't know. That's totally something optional and personally even i like the new envName more! Tought if it does not add much complexity or perf concerns, we can have both :)

This comment has been minimized.

Copy link
@clarkdo

clarkdo Sep 3, 2019

Author Member

Cool, I'll add them all

This comment has been minimized.

Copy link
@clarkdo

clarkdo Sep 3, 2019

Author Member

Added, I'll update readme and doc tonight.

clarkdo added 4 commits Sep 3, 2019
@clarkdo clarkdo requested a review from pi0 Sep 3, 2019
@pi0 pi0 changed the title feat: simplify babel preset config refactor(babel-preset-app): simplify babel preset config Sep 3, 2019
@pi0
pi0 approved these changes Sep 3, 2019
@pi0 pi0 merged commit 2fc73cf into nuxt:dev Sep 3, 2019
9 checks passed
9 checks passed
Semantic Pull Request ready to be squashed
Details
[ci.azure] nuxt.js #20190903.13 succeeded
Details
ci/circleci: audit Your tests passed on CircleCI!
Details
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: lint-app Your tests passed on CircleCI!
Details
ci/circleci: setup Your tests passed on CircleCI!
Details
ci/circleci: test-e2e Your tests passed on CircleCI!
Details
ci/circleci: test-unit Your tests passed on CircleCI!
Details
@pimlie pimlie moved this from Ready to Review to Done in Nuxt v2.10 Sep 4, 2019
@clarkdo clarkdo deleted the clarkdo:babel-config branch Sep 7, 2019
@pi0 pi0 mentioned this pull request Sep 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Nuxt v2.10
  
Done
5 participants
You can’t perform that action at this time.