Make source maps plain objects for use with t.valueToNode#14283
Make source maps plain objects for use with t.valueToNode#14283nicolo-ribaudo merged 2 commits intobabel:mainfrom
t.valueToNode#14283Conversation
|
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/51317/ |
thebanjomatic
force-pushed
the
fix/sourcemap-plain-object
branch
2 times, most recently
from
6dbb4ff to
57a123f
Compare
jridgewell
left a comment
jridgewell
left a comment
There was a problem hiding this comment.
This is a strange usecase, but LGTM.
jridgewell
added
area: sourcemaps
PR: Bug Fix 🐛
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.
thebanjomatic
force-pushed
the
fix/sourcemap-plain-object
branch
from
57a123f to
648c7a3
Compare
nicolo-ribaudo
left a comment
nicolo-ribaudo
left a comment
There was a problem hiding this comment.
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 |
nicolo-ribaudo
changed the title
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. |
github-actions
bot
added
the
outdated
4 participants
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 SourceMapfrom 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/coreto~7.16.0prevents 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: