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

Babel bug: .inputSourceMap must be a boolean, object, or undefined & filename is undefined error fix #138

Merged
merged 30 commits into from
Feb 8, 2019

Conversation

rumanHuq
Copy link

@rumanHuq rumanHuq commented Dec 26, 2018

This is my first pull request ever just FYI :) So, Please go easy on me.

Current @babel/core requires either boolean, or object in inputSourceMap. So putting empty string will throw error (lib/compilers/typescript-compiler.js : 14)

While using @babel/preset-typescript babel.transform requires a filename(lib/compilers/babel-compiler.js: 20), even though the typedef for @babel/core says it's optional. I have given the defaultname "unknown" mentioned in the @types/babel-core file

Ran test to make sure it didn't break anything

line 14. import vue file containing <script lang="ts"> in tests fails due to "node_modules/@babel/core/lib/config/validation/option-assertions.js:110:11)"

line 18:  Even though filename is optional as of their typedef() not providing "filename" throws an error in
"node_modules/@babel/core/lib/config/config-chain.js:431:11"

line 20. It would be nice to upgrade the plugin as well
Copy link
Member

@eddyerburgh eddyerburgh left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, it looks good :)

Can you add a new test that reproduces the issue that's fixed by this change?

@rumanHuq
Copy link
Author

Can you please elaborate the steps? As in how/what should I test?

@eddyerburgh
Copy link
Member

You could create a test that reproduces the issue.

You can see a test here—https://github.com/vuejs/vue-jest/blob/v3/test/Babel.spec.js#L98. It writes plugins to the .babelrc file, then calls jestVue.process. In your case, you would add @babel/preset-typescript to the .babelrc, and then make sure it doesn't error by calling vueJest.process (assuming that it does at the moment). Let me know if you have any other questions

@@ -1,4 +1,4 @@
const babel = require('babel-core')
const babel = require('@babel/core')
Copy link
Member

Choose a reason for hiding this comment

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

This makes this PR a breaking change, can you keep the old babel-core in this PR? You can use babel-core bridge (https://github.com/babel/babel-bridge) if you need functionality of babel 7.

Copy link
Author

Choose a reason for hiding this comment

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

Reverted back.

@rumanHuq
Copy link
Author

rumanHuq commented Dec 29, 2018

Alright, so it was not as easy as I thought it would be. It has to do with babel 7, and the current vue-jest is transpiled with babel 6. Here are the summary of what has been done:

  1. All the related code is upgraded to comply with babel changes.
  2. A test case is setup, where the issue is reproduced.

@rumanHuq rumanHuq changed the title fixes for @babel/preset-typescript users Babel Upgrade fix breaking changes Dec 29, 2018
@rumanHuq
Copy link
Author

#122 should be fixed with this one, along with some other babel 7 related issues which are faced by users

Copy link
Author

@rumanHuq rumanHuq left a comment

Choose a reason for hiding this comment

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

bounced back to babel 6 as per review

@@ -1,4 +1,4 @@
const babel = require('babel-core')
const babel = require('@babel/core')
Copy link
Author

Choose a reason for hiding this comment

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

Reverted back.

@@ -23,7 +23,16 @@ module.exports = function compileBabel (scriptContent, inputSourceMap, inlineCon
babelOptions = Object.assign(babelOptions, babelOverWrites)
}

const res = babel.transform(scriptContent, babelOptions)
Copy link
Member

Choose a reason for hiding this comment

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

Can you revert to the previous code? babel.transform will use babel 6 transform if babel-core is babel 6, and babel 7 if babel-core is @bridge.

Copy link
Author

Choose a reason for hiding this comment

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

I reverted back to original code, except this chunk
image
This to only reproduce the issue in test

Now the snapshot test is failing, can you check the codes for assesment

@rumanHuq rumanHuq deleted the patch-2 branch January 4, 2019 13:37
@rumanHuq rumanHuq restored the patch-2 branch January 9, 2019 12:28
@rumanHuq rumanHuq reopened this Jan 9, 2019
@rumanHuq
Copy link
Author

rumanHuq commented Jan 9, 2019

@eddyerburgh Test provided, snapshot failing. Please check, My test included babel 7, which is why there is snapshot missmatch.

@rumanHuq rumanHuq changed the title Babel Upgrade fix breaking changes Babel bug: .inputSourceMap must be a boolean, object, or undefined Jan 9, 2019
@rumanHuq rumanHuq changed the title Babel bug: .inputSourceMap must be a boolean, object, or undefined Babel bug: .inputSourceMap must be a boolean, object, or undefined & filename is undefined error fix Jan 9, 2019
@DotCoyote
Copy link

For anyone who encounters this error too: After several hours I found a fix/workaround for this problem:

tsconfig.json:

{
  "compilerOptions": {
    [...]
    "allowSyntheticDefaultImports": true,

This is the important setting.

@kermorgant
Copy link

Same error happened to me, and in my case, I solved it by adding "sourceMap": true in tsconfig.json

lib/process.js Outdated
const vueJestConfig = getVueJestConfig(jestConfig)

var parts = vueCompiler.parseComponent(src, { pad: true })

if (parts.script && parts.script.src) {
parts.script.content = fs.readFileSync(join(filePath, '..', parts.script.src), 'utf8')
}

const result = processScript(parts.script, vueJestConfig, filePath)
const result = processScript(parts.script, vueJestConfig, filePath, babelTestObj)
Copy link
Member

Choose a reason for hiding this comment

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

Can you please remove the babelTestObj from the code

Copy link
Author

Choose a reason for hiding this comment

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

done

cache.flushAll()
clearModule.all()
})
test('babel 7 configuration expects filename [default: "unknown"] to compile', () => {
Copy link
Member

Choose a reason for hiding this comment

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

In this case, since it's a small change and difficult to test, you can remove these tests and we'll leave it untested

Copy link
Author

Choose a reason for hiding this comment

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

done

@eddyerburgh
Copy link
Member

So since the changes to code are relatively small, and we'll realase v4 soon, it's OK to add this code without any tests. Can you please put an inline comment above the code to add the filename, and above the ternary where you assign the input map, to describe why you made the changes?

@@ -17,11 +17,15 @@ module.exports = function compileBabel (scriptContent, inputSourceMap, inlineCon
}

const babelOptions = Object.assign(sourceMapOptions, babelConfig)
if (!babelOptions.filename) babelOptions.filename = 'unknown'
Copy link
Member

Choose a reason for hiding this comment

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

Can you remove this line please

Copy link
Member

@eddyerburgh eddyerburgh left a comment

Choose a reason for hiding this comment

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

Thanks for the changes :)

@eddyerburgh eddyerburgh merged commit c44e618 into vuejs:v3 Feb 8, 2019
@eddyerburgh
Copy link
Member

This has been released in 3.0.3 👍

@mattgrande
Copy link

@eddyerburgh - I'm not seeing 3.0.3 on NPM yet. Is there a delay on NPM's side?

@eddyerburgh
Copy link
Member

No sorry, the publish had failed on my end. Just published now

@rumanHuq rumanHuq deleted the patch-2 branch February 9, 2019 07:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants