Use the correct context when re-using a cached NodePath
#12331
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nicolo-ribaudo
added
PR: Bug Fix 🐛
nicolo-ribaudo
commented
Nov 7, 2020
| @@ -6,7 +6,7 @@ function _iterableToArrayLimit(arr, i) { if (typeof Symbol === "undefined" || !( | |||
|
|
|||
| function _arrayWithHoles(arr) { if (Array.isArray(arr)) return arr; } | |||
|
|
|||
| function _createForOfIteratorHelper(o, allowArrayLike) { var it; if (typeof Symbol === "undefined" || o[Symbol.iterator] == null) { if (Array.isArray(o) || (it = _unsupportedIterableToArray(o)) || allowArrayLike && o && typeof o.length === "number") { if (it) o = it; var i = 0; var F = function F() {}; return { s: F, n: function n() { if (i >= o.length) return { done: true }; return { done: false, value: o[i++] }; }, e: function e(_e) { throw _e; }, f: F }; } throw new TypeError("Invalid attempt to iterate non-iterable instance.\nIn order to be iterable, non-array objects must have a [Symbol.iterator]() method."); } var normalCompletion = true, didErr = false, err; return { s: function s() { it = o[Symbol.iterator](); }, n: function n() { var step = it.next(); normalCompletion = step.done; return step; }, e: function e(_e2) { didErr = true; err = _e2; }, f: function f() { try { if (!normalCompletion && it["return"] != null) it["return"](); } finally { if (didErr) throw err; } } }; } | |||
| function _createForOfIteratorHelper(o, allowArrayLike) { var it; if (typeof Symbol === "undefined" || o[Symbol.iterator] == null) { if (Array.isArray(o) || (it = _unsupportedIterableToArray(o)) || allowArrayLike && o && typeof o.length === "number") { if (it) o = it; var i = 0; var F = function F() {}; return { s: F, n: function n() { if (i >= o.length) return { done: true }; return { done: false, value: o[i++] }; }, e: function e(_e2) { throw _e2; }, f: F }; } throw new TypeError("Invalid attempt to iterate non-iterable instance.\nIn order to be iterable, non-array objects must have a [Symbol.iterator]() method."); } var normalCompletion = true, didErr = false, err; return { s: function s() { it = o[Symbol.iterator](); }, n: function n() { var step = it.next(); normalCompletion = step.done; return step; }, e: function e(_e3) { didErr = true; err = _e3; }, f: function f() { try { if (!normalCompletion && it["return"] != null) it["return"](); } finally { if (didErr) throw err; } } }; } | |||
There was a problem hiding this comment.
I'm not sure why the tmp var name is changing here.
|
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/31959/ |
nicolo-ribaudo
commented
Nov 7, 2020
| @@ -9,8 +11,6 @@ var _args = _interopRequireDefault(require("utils/url/args")); | |||
|
|
|||
| function _interopRequireDefault(obj) { return obj && obj.__esModule ? obj : { "default": obj }; } | |||
|
|
|||
| function _typeof(obj) { "@babel/helpers - typeof"; if (typeof Symbol === "function" && typeof Symbol.iterator === "symbol") { _typeof = function _typeof(obj) { return typeof obj; }; } else { _typeof = function _typeof(obj) { return obj && typeof Symbol === "function" && obj.constructor === Symbol && obj !== Symbol.prototype ? "symbol" : typeof obj; }; } return _typeof(obj); } | |||
There was a problem hiding this comment.
Ironically, this test was about another context caching bug (#6057), but this change has nothing to do with that bug.
|
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 e7ce1ba:
|
JLHwung
reviewed
Nov 7, 2020
| @@ -43,6 +43,10 @@ node "$PWD"/scripts/integration-tests/utils/bump-babel-dependencies.js | |||
| fs.writeFileSync('./package.json', JSON.stringify(pkg, null, 2)); | |||
| " | |||
|
|
|||
| # https://github.com/babel/babel/pull/12331 - This test is fixed in @babel/traverse 7.12.7, | |||
| # so it will fail with older versions. We can disable it. | |||
| (cd packages/babel-plugin-transform-modules-systemjs/test/fixtures/regression/; mv issue-12329 .issue-12329) | |||
There was a problem hiding this comment.
What about supporting minBabelVersion in fixture runner 😄
There was a problem hiding this comment.
Only if this problem comes up more than once 😛
existentialism
approved these changes
Nov 10, 2020
|
after this pr, my project crash ... |
This comment has been minimized.
This comment has been minimized.
2 tasks
|
|
|
@ycjcl868 I'm not sure how it is related to this PR, could you open a new issue? 🙏 |
|
When I reset this line code, the build is passed.
|
|
Thanks, investigating. |
|
@ycjcl868 It's after midnight in Hangzhou, have a good night. We are investigating, a patch will be released today. |
1 task
github-actions
bot
added
the
outdated
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When re-using a cached path, by default we didn't update the context/opts. This means that, if I traverse a node with
{ noScope: true }and then re-traverse it with{ noScope: false }, the second one will still havenoScope: truebecause it re-uses the cached path with the original context.The error in #12329 happens because we introduced
noScope: truein a traversal used by the SystemJS plugin inProgram:enter, and that leaked in to the class properties plugin, causingconstructor.scopeto be undefined in thehelper-class-featurespackage.This is technically a breaking change (we are changing the default value of a parameter in the public API), but I couldn't find anyone online using that parameter and I'm not sure why anyway would not wont it to use the correct context.