Disallow new import(x) and import(x,)
#9313
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
Member
nicolo-ribaudo
commented
Jan 11, 2019
nicolo-ribaudo
commented
| Q | A |
|---|---|
| Fixed Issues? | |
| Patch: Bug Fix? | 👍 |
| Major: Breaking Change? | |
| Minor: New Feature? | |
| Tests Added + Pass? | Yes, 100% ✔️ Test262 for dynamic import |
| Documentation PR Link | |
| Any Dependency Changes? | |
| License | MIT |
nicolo-ribaudo
added
PR: Bug Fix 🐛
|
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/9758/ |
nicolo-ribaudo
added
PR: Spec Compliance 👓
| if ( | ||
|
|
||
| if (node.callee.type === "Import") { | ||
| this.raise(node.callee.start, "import(...) can't be constructed"); |
There was a problem hiding this comment.
Feel like Chrome's error here is clearer?
Cannot use new with import
existentialism
approved these changes
Jan 11, 2019
|
I don't get how this was not handled as a breaking change, at least it broke my build 😅 (Still great work you're doing with Babel though! I don't know where web development would be without Babel. Thank you for constantly making it better!) For anyone using ESLint in combination with the -const MyModule = () => import(
- /* webpackChunkName: "my-module" */ '../some/very/very/very/very/very/very/very/very/very/long/path/to/module.js',
-);
+// eslint-disable-next-line max-len
+const MyModule = () => import(/* webpackChunkName: "my-module" */ '../some/very/very/very/very/very/very/very/very/very/long/path/to/module.js'); |
|
Sorry to have broken your build, but every bug fix is breaking is someone relies on the buggy behavior 😅 |
|
@babel/babel We might consider allowing trailing commas until tc39/proposal-dynamic-import#67 is closed, if disallowing them breaks code generated by eslint. OTOH, Prettier does the right thing: import("fooooooooooooooooooooooooooooooooooooooooooo" +
"oooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooo");
fn(
"fooooooooooooooooooooooooooooooooooooooooooo" +
"oooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooo",
);so we could just fix the |
dev6james
pushed a commit
to dev6james/babel
that referenced
this pull request
Jun 18, 2019
* Disallow `new import(x)` and `import(x,)` (babel#9313) * Disallow "new import(...)" * Disallow trailing comma inside dynamic import * Rename test * Update error message * add sponsor [skip ci] * feature: parser layout * feature: add if predicator tests * support mutilples types when clauses * fix: pattern matching clause * feature: add modules of transform plugin and syntax plugin * add project structure * add definitions to babel-types * fix array match pattern * add match guard * support nested pattern, optimize pattern test * support rest element * support variable match * add tests of block * add generators of case statement * wip nested test * wip fix bogus use of Object.is Spec says: when {x: 1} -> ... // matches if `input` can do ToObject and `input.x` is 1 Sure, that sounds good. That in turn means: https://tc39.github.io/ecma262/#sec-toobject anything that isn't undefined or null. Do that. * Fill out more of the "nested" test. * Begin adding logic tests. * Crudely log the offending code on syntax errors. * make that error-handling a bit less crude * Fix the syntax error, superficially. Even with this fix, I think this is not the right way we should be handling this list of IDs. * Fix related bug too, again superficially. * add a bunch more test cases * more specific about known failures * Fix array lengths, kind of verbosely. * clarify the "undefined" situation * Organize tests more. * build a list * Use tagged-template syntax. * Add names to plugins. * Start new implementation of transform; `jest matching -t prim` works! * Include `{x: 1}` pattern in parser tests * implement obj match -- `-t object` passes * Include `[a, ...rest]` pattern in parser tests * implement array match -- now `-t array` works too * Add undefined-check on object properties * Implement match guards. Now passes all exec tests! * Convert match-guard tests to exec form. There's only one test case here, and we happen to have already covered it in an exec test. * Implement array rest elements; convert tests to exec. * Convert "block" test to exec * Convert remaining input/output tests to exec. * Add (failing, commented-out) tests for object rest elements. * Give nested-pattern test a more specific name. * Rename "block" tests to "scoping". * Add tests for hoisting, and continue/break. * Add some tests for nested case statements -- one failing! * Fix nesting: use less visitor pattern * Cut a question-todo: yep, generateUid checks for labels * Cut some silly redundancy, oops * Factor out some common small helpers. * Refactor recursive rewriter to a class, for state. * Factor out main state-interaction patterns. * Consolidate outermost logic. * Name a variable better: caseLabel. * Push when-clause toplevel into class. * Further simplify top-level logic. * Fix a silly subtle bug; add assert + test to keep it fixed. Found in the first place while rereading the relevant bit of code. * Rewrite labels to keep inner breaks/continues unaware of our transform. * Expand continue/break tests; and find a bug! * Fix bug when rewriting several breaks/continues; enable test. * Report better errors. * Enable a no-longer-failing test. This was fixed a while ago, before the v2 transform code, even. * Add still more tests for break/continue handling. * Move label-rewriting to its own file. It's pretty independent of the rest of the logic, with a nice narrow interface, and is sort of a side detail vs. the main point. * gitattributes: Suppress output.json files when showing diffs. * Add pattern-matching to "stage 1" preset in @babel/standalone. * Mark one test case as for Node 8+ only. This failed in CI on Node 6; and the issue seems likely orthogonal to this plugin. * Try another way to put plugin in standalone preset-stage-1. * generateUid: Only try the name `_temp` once. We were trying the names _temp _temp _temp2 _temp3 ... Skip the first one -- so it's just _temp _temp2 _temp3 ... (Alternatively, the hyper-optimized way would be like this: _temp _temp0 _temp1 _temp2 ... That's what C++ compilers do in name-mangling, to make it a little less common to have to spend the extra character for `_temp10`. But of course that breaks almost every fragile, output-dependent test case in the codebase... 182/8641 tests in 32/134 suites, by my count.) * traverse: Add some known-failing tests on label collisions. Labels work quite differently from identifier bindings in how they collide. It's actually perfectly fine to have several of them as siblings: function f() { loop: do { continue loop; } while (0); loop: while (1) { break loop; }; loop: if (1) { break loop; /* :-P */ } } On the other hand, it's *not* permitted to have one as an ancestor of another: function f() { loop: do { loop: while (1) { // Error! break loop; // After all -- what should this mean? } console.log("would this run?"); } while (0); } So we'll need to handle them differently. This commit adds a couple of unit tests, and also a more end-to-end test with a transform, that demonstrate the issue. * traverse: New algorithm to avoid label collisions. This fixes (and un-comments) the tests added in the previous commit. We don't do anything to track labels on modifications, just like the previous algorithm didn't. Instead, like before, we rely on the rather sledgehammer solution that any name gotten from `generateUid` goes into `getProgramParent().uids`... and if a transform is inserting labels that it *hasn't* gotten from `generateUid`, it's probably in trouble already. * pattern-matching: Simplify by using labeled, non-loop, blocks. We've been using `continue` to break out of a non-match, like this: do { // ... if (!/* match */) continue; // ... /* body */ } while (0); This works fine, but required adding some complexity because any `continue` or `break` statement inside the body, if it didn't refer to an explicit label, required rewriting to avoid having the new `do`-loop capture it. If instead we use `break LABEL`, then we can have the outer block be *not a loop*: _label: { // ... if (!/* match */) break _label; // ... /* body */ } Now an unlabelled `break` or `continue` is unaffected, so the problem is reduced to generating a unique label to use -- which is a general problem we have general infrastructure for, in `scope.generateUid`. So do that. * pattern-matching: Implement object rest properties. For this, I experiment with a different implementation strategy for object patterns. Instead of translating a pattern like `{x: 1, y}` to code like (edited slightly for clarity): const _x = _val.x; if (_x !== 1) break _caseInner; const y = _val.y; if (y === void 0) break _caseInner; we emit code that uses a destructuring bind, like this: const { x: _x, y } = _val; if (_x !== 1) break _caseInner; if (y === void 0) break _caseInner; There are two motivations for this experiment. First, on the implementation side, it makes it very straightforward to translate a pattern like `{x: 1, ...y}`: const { x: _x, ...y } = _val; if (_x !== 1) break _caseInner; whereas the corresponding code in the first style would have to do a lot more. Second, on the semantics side: it seems highly desirable for a when-clause like `when { x } ->` to have semantics as closely matched as possible to `const { x } = val;`, differing only in ways that are essential to its pattern-matching purpose. So if there are subtle semantic differences lurking in this implementation (and I haven't yet dug deeply to learn if there are), they'll be candidates for changing the spec, and if nothing else will deserve explicit discussion there. * pattern-matching: Delete parser tests covered better elsewhere. The areas these cover are more thoroughly covered now in the exec tests, which are also more readable and more robust to internal changes. * pattern-matching: Start adding parser tests for SyntaxErrors. First, for object rest patterns only permitting an identifier pattern. * pattern-matching: Transform a test more, for Node 6's sake. Otherwise in CI we get a syntax error on an object rest property. * pattern-matching: Allow function declarations in when-blocks. Once allowed in parsing, they transform to exactly the specified behavior! In addition to enabling the relevant test, add a contrasting one for clarity. * pattern-matching: Make premature object rest-pattern a syntax error. * pattern-matching: Consolidate object-pattern parsing as one loop. This seems a bit easier to follow. As a bonus, use `checkCommaAfterRest` for nicer error messages. * pattern-matching: Unpack object-pattern translation code a bit. This is a bit longer, but more explicit and will be more amenable to adding another feature -- namely, initializers! 🎺 * pattern-matching: Allow non-identifier keys, just like destructuring. There's no obvious reason we can't support this just as well in pattern-matching as in normal destructuring -- so match the semantics. * pattern-matching: Don't let an object pattern match, say, a string. See TODO comment -- I'm not certain this is ultimately the right behavior, but I think it probably is, and certainly the choice should be made explicitly. * pattern-matching: Implement initializers for object properties.
lock
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.