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

Fix e2e vue-cli Babel 8 tests #13050

Merged
merged 7 commits into from Mar 25, 2021
Merged

Conversation

nicolo-ribaudo
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo commented Mar 24, 2021

Q                       A
Fixed Issues?
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT

The failure was introduced by #12989.

@codesandbox-ci
Copy link

codesandbox-ci bot commented Mar 24, 2021

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 276d3b9:

Sandbox Source
babel-repl-custom-plugin Configuration
babel-plugin-multi-config Configuration

@babel-bot
Copy link
Collaborator

babel-bot commented Mar 24, 2021

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/44712/

Comment on lines 103 to 113
-test('dep from node_modules should been transpiled when matched by transpileDependencies', async () => {
+test.skip('dep from node_modules should been transpiled when matched by transpileDependencies', async () => {
await project.write(
'vue.config.js',
`module.exports = { transpileDependencies: ['external-dep', '@scope/external-dep'] }`
@@ -84,7 +84,7 @@ test('dep from node_modules should been transpiled when matched by transpileDepe
expect(await readVendorFile()).toMatch('return "__SCOPE_TEST__"')
})

-test('dep from node_modules should been transpiled when transpileDependencies is true', async () => {
+test.skip('dep from node_modules should been transpiled when transpileDependencies is true', async () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

I couldn't figure out how to make these two tests pass, so I disabled them for now.

The main problem is that Vue's Babel presets directly calls getTargets(), so even if we specify the top-level targets option it's ignored.

The proper way to update this test would be to pass to Vue's Babel preset ie: 9 as targets, but I don't know how to modify its options.

Copy link
Member

Choose a reason for hiding this comment

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

Reading through https://cli.vuejs.org/config/#babel and the setup think we just create a config file at https://github.com/vuejs/vue-cli/blob/89af6c50c2e559487d5ac24071a1f8589f9c026b/packages/%40vue/cli-plugin-babel/__tests__/transpileDependencies.spec.js#L19, worked for me

  await project.write(
    "babel.config.js",
    `module.exports = { presets: [
      ["@vue/babel-preset-app", { targets: { ie: 9 } }]
    ]}`
  );

if [ "$BABEL_8_BREAKING" = true ] ; then
# Babel 8 defaults to "default, not ie 11", but this breaks vue-cli's tests
# which expect different ES features to be compiled.
patch -p1 < "$PWD"/../../patches/vue-cli-babel-8.patch
Copy link
Member Author

Choose a reason for hiding this comment

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

Ended up doing a patch, it feels less hacky than a regexp.

Copy link
Member

Choose a reason for hiding this comment

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

sounds good, noticed vue-cli had one too!

@nicolo-ribaudo
Copy link
Member Author

cc @sodatea This is an expected Babel 8 breaking change and I don't think that there is nothing you need to do in vue-cli now (after the Babel 8 release, you might choose to override Babel's defaults).

I'm pinging you just to let you know about this!

@nicolo-ribaudo nicolo-ribaudo marked this pull request as ready for review March 25, 2021 17:00
Co-authored-by: Henry Zhu <hi@henryzoo.com>
const a = new Map()
`.trim(), {
babelrc: false,
+ targets: { ie: 9 },
Copy link
Contributor

@JLHwung JLHwung Mar 25, 2021

Choose a reason for hiding this comment

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

Q: Why should we patch given that the preset has specified { node: 'current' } in targets option?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh right, it's not necessary. It was my first attempt at fixing these tests, and since it reduced the number of failure I didn't realize it's not what caused them.

@nicolo-ribaudo
Copy link
Member Author

There are some failures but they are all unrelated, there is probably some problem with the registry.

@nicolo-ribaudo
Copy link
Member Author

@nicolo-ribaudo nicolo-ribaudo merged commit 8c445e6 into babel:main Mar 25, 2021
@nicolo-ribaudo nicolo-ribaudo deleted the e2e-vue-fix branch March 25, 2021 18:23
@sodatea
Copy link
Contributor

sodatea commented Mar 26, 2021

Cool! Thanks for letting me know!

@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Jun 25, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: tests outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: Fixes failing main
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants