-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
Make source maps plain objects for use with t.valueToNode
#14283
Make source maps plain objects for use with t.valueToNode
#14283
Conversation
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/51317/ |
6dbb4ff
to
57a123f
Compare
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.
This is a strange usecase, but LGTM.
When the sourcemaps were switched to use "@ampproject/remapping", the type of the inputSourceMap that gets passed around changed from being a plain js object to a `class SourceMap` fromthe remapping package. This causes issues with the @babel/types valueToNode function because that function is defined to throw for objects that aren't plain-objects. In practice I was seeing this error after upgrading babel when running code- coverage, and I saw a similar error reported here: vuejs/vue-jest#450 before deciding to try to track the error down myself.
57a123f
to
648c7a3
Compare
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.
This feels hacky/strange, but I'm ok with it since it's to restore backward compatibility. However, we might consider reverting this in Babel 8 since it's an unnecessary clone for 99.9% of the possible use cases, and you will then need to do t.valueToNode({ ...sourceMap })
.
May I ask why you pass source maps to valueToNode
?
I think no one in the team uses Windows, so our configuration might need to be fixed to work with |
t.valueToNode
@nicolo-ribaudo I agree this maybe isn't ideal. I just tried to track that down. I don't own/maintain any of the projects that were involved in this error, this just came up when I was running jest for code coverage. It looks like the sourcemap is being grabbed from And istanbul-lib-instrument is then calling valueFromNode on the coverage data which includes the inputSourceMap here I think under the assumption that this.cov.toJSON() actually converts to json: It looks like there is already an issue now in |
@nicolo-ribaudo Assuming this babel-plugin-istanbuil PR gets merged and released, we can probably revert this PR. Sorry for not finding that sooner as I think its definitely the more appropriate fix, it was much easier when dealing with unfamiliar code to find the immediate cause of the error, and where the sourcemap was created, but less obvious tracking down everything that happened to the sourcemap along the way. |
No problem, I think it's still appropriate to have the fix here because we caused the regression. We can revert in Babel 8, but I don't think it's terrible. |
When the sourcemaps were switched to use "@ampproject/remapping", the type
of the inputSourceMap that gets passed around changed from being a plain js
object to a
class SourceMap
from the remapping package. This causes issueswith the @babel/types valueToNode function because that function is defined
to throw for objects that aren't plain-objects.
In practice I was seeing this error after upgrading babel when running code-
coverage, and I saw a similar error reported here: vuejs/vue-jest#450 before
deciding to try to track the error down myself.
This appears to be broken since
7.17.0
(pinning@babel/core
to~7.16.0
prevents the issue from occurring).This is my first time contributing to babel and I had issues getting the project to build locally with an error: