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(NodePart): factory method created to help us when creating NodePart introduced due to scope hoisting problem with ParcelJS #954

Conversation

Stradivario
Copy link

Original Issue ParcelJS

Description

The problem here is that when compiled this part of code remains the same without export referencing him to the module scope

Typescript

itemPart = new NodePart(this.options);

Javascript

itemPart = new NodePart(this.options);

Instead it should be

itemPart = new exports.NodePart(this.options);

The case here is that i am getting the following error

Uncaught ReferenceError: NodePart is not defined

Type of change

  • Bug fix
  • Not introducing any existing change with existing API

Checklist:

  • Implement factory method creating new NodeParts

Example repository of the problem:

https://github.com/Stradivario/parcel-scope-hoisting

Regards! :)

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@Stradivario
Copy link
Author

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@Stradivario Stradivario force-pushed the factory-method-for-node-part-creation-scope-hoisting-fix branch 2 times, most recently from bd5eae0 to 6008292 Compare June 28, 2019 15:10
…rt, introduced due to scope hoisting problem with ParcelJS
@Stradivario
Copy link
Author

I think this issue is related with how typescript compile this part of the code.
If it is working with exports.NodePart manually added why TS decide to not catch this reference ?

@justinfagnani
Copy link
Collaborator

This isn't a bug with lit-html, and adds unnecessary lines of code. We should wait for the bugfix in Parcel.

@Stradivario
Copy link
Author

This isn't a bug with lit-html, and adds unnecessary lines of code. We should wait for the bugfix in Parcel.

I understand!

What i cannot understand is why typescript doesn't put exports.NodePart in front since it does it in some other cases ? Is this a bug with Typescript ?

Regards!

@Stradivario
Copy link
Author

Stradivario commented Jun 28, 2019

Other strange thing is this

When typescript compile es6 commonjs class for example:

export class NodePart {}

Will be compiled to:

"use strict";
Object.defineProperty(exports, "__esModule", { value: true });
class NodePart {
}
exports.NodePart = NodePart;
//# sourceMappingURL=index.js.map

What it works ?!?

"use strict";
Object.defineProperty(exports, "__esModule", { value: true });
exports.NodePart = class NodePart {
}
//# sourceMappingURL=index.js.map

Do you know why they decided this approach ?
Since even with that simple change everything work as expected without this pull request ?

@Stradivario
Copy link
Author

@justinfagnani closing since Parcel Team confirmed that it is a bug in the renaming when tree shaking the whole bundle.

parcel-bundler/parcel#3172 (comment)

Thank you for your fast answer and correct resolution of the problem!

Regards!

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.

None yet

3 participants