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

Fix generated code order when using TypeScript, ES6 and decorators #7293

Conversation

thewilkybarkid
Copy link
Contributor

↪️ Pull Request

This fixes the problem I mentioned in #6676 (comment). I'm not sure I understand the actual problem, but it's something to do with the hoisting difference between var and let, and decorators. (I didn't manage to recreate it without decorators.)

Moving the call to parcelHelpers.export() to the end (i.e. after the let) seems to resolve the problem.

💻 Examples

I've managed to reproduce the problem in https://github.com/thewilkybarkid/parcel-access-before-initialization, where you can see the compilation result and the failure. (It's basically the same as the test case in the PR.)

🚨 Test instructions

✔️ PR Todo

  • Added/updated unit tests for this change
  • Filled out test instructions (In case there aren't any unit tests)
  • Included links to related issues/PRs

@height
Copy link

height bot commented Nov 12, 2021

Link Height tasks by mentioning a task ID in the pull request title or commit messages, or description and comments with the keyword link (e.g. "Link T-123").

💡Tip: You can also use "Close T-X" to automatically close a task when the pull request is merged.

@thewilkybarkid
Copy link
Contributor Author

Only ran a subset of tests locally (as they take so long); will investigate those.

@thewilkybarkid thewilkybarkid marked this pull request as ready for review November 12, 2021 20:27
@basher
Copy link

basher commented Nov 18, 2021

Thanks @thewilkybarkid for doing this.

Hopefully it will fix #6676 so I can finally upgrade to Parcel 2 and still support certain legacy browsers!

@mischnic
Copy link
Member

Not sure if this entirely safe to do, circular imports might be problematic and also:

#7016:

This updates our helpers to skip keys that are already defined when performing a wildcard re-export. This is safe because we hoist named exports to the top of the module, so by the time the helper is called, all named exports are already defined.

@thewilkybarkid
Copy link
Contributor Author

Are you able to write a test that breaks?

@thewilkybarkid
Copy link
Contributor Author

thewilkybarkid commented Nov 22, 2021

@mischnic As far as I'm aware the export order is still fine (it's just at the end of the module). The test I added contains circular imports too, but maybe you had a different idea.

I've just tried this out on a real project and it works fine, but still has the same problem with scope hoisting enabled (as the order is changed back)...

@mischnic
Copy link
Member

mischnic commented Nov 22, 2021

That just came to mind because we had problems with this in the past as well. I haven't had time yet to try and test such a situation

@devongovett
Copy link
Member

Here's an example:

a.mjs

import { c } from "./b.mjs";
console.log(c);

b.mjs

export const foo = "foo";
export { c } from "./c.mjs";

c.mjs

import { foo } from "./b.mjs";
export const c = "c:" + foo;

When run, this should throw "Cannot access 'foo' before initialization" (as it does when running the modules natively in Node or browsers). However, after applying this patch, it logs "c:undefined".

@thewilkybarkid
Copy link
Contributor Author

Thanks @devongovett, I've opened #7350 with that test case. I'll move this back to draft and will keep looking.

@thewilkybarkid thewilkybarkid marked this pull request as draft November 23, 2021 09:55
@thewilkybarkid
Copy link
Contributor Author

I've just been able to find angular/angular@401ef71 and microsoft/TypeScript#27519 which seem to describe this problem: it's a TypeScript limitation. I'm not sure how it was working in Parcel 2.0.0-beta1, but I'll look more thoroughly at the generated output there.

Not sure what to do with this problem now though.

(The project where I've had this problem is inherited, and I've never used decorators before as they are known to be experimental. This gives me the excuse to remove them.)

@devongovett
Copy link
Member

I think the reason it may have "worked" in a previous beta is because we weren't spec compliant yet. Are you sure this circular dependency works with normal TSC or Babel?

@thewilkybarkid
Copy link
Contributor Author

Are you sure this circular dependency works with normal TSC or Babel?

Seems to work with TSC: I've expanded https://github.com/thewilkybarkid/parcel-access-before-initialization to show a failure with Parcel but success with TSC.

thewilkybarkid added a commit to PREreview/prereview that referenced this pull request Nov 25, 2021
Decorators are an experimental feature in TypeScript, which is known to have issues. Changes in Parcel means that it can no longer build the app, so their use is blocking updating Parcel to a stable release.

This change removes the ORM decorators in favour of defining entities programmatically. I'm hoping that the tests and migration-creating script are enough to have caught any mistakes, but given that areas of the app remain untested, it's hard to be sure.

Also, this change has to disable Parcel's scope hoisting, as it renames the entity classes. The rename wasn't a problem when using decorators as the generated code saw the class name left unchanged. Now, however, the ORM breaks if they are changed.

Refs #399, parcel-bundler/parcel#7293 (comment), microsoft/TypeScript#27519
@thewilkybarkid thewilkybarkid deleted the typescript-es6-decorators branch December 8, 2021 15:34
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.

4 participants