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

Support ESM #247

Merged
merged 13 commits into from
Sep 7, 2023
Merged

Support ESM #247

merged 13 commits into from
Sep 7, 2023

Conversation

FZambia
Copy link
Member

@FZambia FZambia commented Sep 1, 2023

Support ESM in addition to CommonJS. Relates #245

@FZambia FZambia mentioned this pull request Sep 3, 2023
@HJK181
Copy link

HJK181 commented Sep 6, 2023

Do you have any timeline for when this is going to be released?

@FZambia
Copy link
Member Author

FZambia commented Sep 6, 2023

Will try during this week, this is quite a huge change - need one more iteration over the changes. Probably you can test out this branch meanwhile?

@HJK181
Copy link

HJK181 commented Sep 6, 2023

Tested: it's no longer throwing the error described in #245
However, the Polymer dev server throws:

Error SyntaxError: This experimental syntax requires enabling the parser plugin: 'classProperties' (586:11)
    at Parser.raise (/someDir/node_modules/polymer-cli/node_modules/babylon/lib/index.js:776:15)
    at Parser.expectPlugin (/someDir/node_modules/polymer-cli/node_modules/babylon/lib/index.js:2084:18)
    at Parser.parseClassProperty (/someDir/node_modules/polymer-cli/node_modules/babylon/lib/index.js:4920:12)
    at Parser.pushClassProperty (/someDir/node_modules/polymer-cli/node_modules/babylon/lib/index.js:4884:30)
    at Parser.parseClassMemberWithIsStatic (/someDir/node_modules/polymer-cli/node_modules/babylon/lib/index.js:4817:14)
    at Parser.parseClassMember (/someDir/node_modules/polymer-cli/node_modules/babylon/lib/index.js:4754:10)
    at Parser.parseClassBody (/someDir/node_modules/polymer-cli/node_modules/babylon/lib/index.js:4709:12)
    at Parser.parseClass (/someDir/node_modules/polymer-cli/node_modules/babylon/lib/index.js:4659:10)
    at Parser.parseStatementContent (/someDir/node_modules/polymer-cli/node_modules/babylon/lib/index.js:3990:21)
    at Parser.parseStatement (/someDir/node_modules/polymer-cli/node_modules/babylon/lib/index.js:3962:17) {
  pos: 18470,
  loc: Position { line: 586, column: 11 },
  missingPlugin: [ 'classProperties' ]

But the application still works, guess you can ignore this as it should not be related to ESM but specifically to Polymer.

@HJK181
Copy link

HJK181 commented Sep 6, 2023

Unfortunately, I can't work with this release as the build of my application fails

file:///someDir/src/sh-deployment.js(9,28) error [could-not-load] - Unable to load import: This experimental syntax requires enabling the parser plugin: 'classProperties' (586:11)
error:  Uncaught exception: Error: 1 error(s) occurred during build.
error:  Error: 1 error(s) occurred during build.
    at BuildAnalyzer._done (/someDir/node_modules/polymer-cli/node_modules/polymer-build/lib/analyzer.js:277:36)
    at BuildAnalyzer.<anonymous> (/someDir/node_modules/polymer-cli/node_modules/polymer-build/lib/analyzer.js:238:26)
    at Generator.next (<anonymous>)
    at fulfilled (/someDir/node_modules/polymer-cli/node_modules/polymer-build/lib/analyzer.js:17:58)

Line 9 is this import import { Centrifuge } from 'centrifuge';

It seems like you are using some features that the other packages I'm using don't?! Maybe you can test it with the provided sample in #245 by running npm run build

@FZambia
Copy link
Member Author

FZambia commented Sep 6, 2023

You mean that polymer serve works fine, but you are getting error when trying to build your app right?

@HJK181
Copy link

HJK181 commented Sep 6, 2023

You mean that polymer serve works fine, but you are getting error when trying to build your app right?

Yes, that's correct. In our real application polymer serve works with the ESM import. The development server just throws that classProperties error. However, I've tried to to also link the esm branch in the sample application I've provided, and there I'm still getting SyntaxError: The requested module '../node_modules/centrifuge/build/index.js' does not provide an export named 'Centrifuge' (at my-view1.js:12:10). It would be nice if you can test the esm branch against my sample application. Please do a git pull in the repo, as I updated the Ploymer-Cli version.

@FZambia
Copy link
Member Author

FZambia commented Sep 6, 2023

I suppose you can extend app configuration by enabling classProperties support - a quickly googled it and it seems it's a matter of Babel configuration.

I can check this myself later - hopefully later today.

@HJK181
Copy link

HJK181 commented Sep 6, 2023

Could make polyer serve work in the sample application by regenerating package-lock.json. Please also pull that change. If you do npm start the view renders now without any error. However, npm run build produces the classProperties error.

@HJK181
Copy link

HJK181 commented Sep 6, 2023

I suppose you can extend app configuration by enabling classProperties support - a quickly googled it and it seems it's a matter of Babel configuration.

I can check this myself later - hopefully later today.

Isn't this a babel configuration inside the Polymer analyzer package? Can I override that from within my application? I'll try it too.

@HJK181
Copy link

HJK181 commented Sep 6, 2023

You are right, I could solve it by installing "@babel/plugin-proposal-class-properties" and adding it to my own ``.babelrc.
Pushed the fix to my demo which is now working fine, both polymer serve and `polymer build`. Thanks for your first-class support.

@FZambia
Copy link
Member Author

FZambia commented Sep 6, 2023

Great, thanks! Let me then check out your test repo with latest changes, iterate over changes one more time, I will then release centrifuge-js v5-beta version and let you know here - so you can try it in your real app. And then we can proceed with release.

@HJK181
Copy link

HJK181 commented Sep 6, 2023

Hi @FZambia Sorry for spamming, but while I've tried to transfer the supposedly working babel configuration to my real project, I realized that it does not work. Installing @babel/plugin-proposal-class-properties npm resolved centrifuge-js from the remote repository instead of my linked local directory where I checked out the esm branch.

After linking the local directory again, the classProperties error is still there. I'd suggest you test it by yourself with my demo application. I've tried several Babel configuration files listed here: https://babeljs.io/docs/configuration but none of them were picked up.

Some interesting links:
Polymer/tools#3360 (comment)
https://groups.google.com/g/polymer-dev/c/kd2zgAt6-GE
Polymer/polymer#4848 (comment)

@FZambia
Copy link
Member Author

FZambia commented Sep 6, 2023

After linking the local directory again, the classProperties error is still there. I'd suggest you test it by yourself with my demo application.

But you said demo app works OK now? So I am not sure how I can test it in this case since problem only reproduces in your real app.

@HJK181
Copy link

HJK181 commented Sep 6, 2023

The demo app only worked because after I ran npm i @babel/plugin-proposal-class-properties npm also installed centriguges-js 4.1.0 again, overriding my previously linked local directory where I checked out your esm branch. After linking the local directory, it was broken.

For you this means, just pull my latest changes, run npm i and afterward npm run build, which should produce the error.

@HJK181
Copy link

HJK181 commented Sep 6, 2023

I fear that w/o changing some code in your repo I will not be able to use it with Polymer. I could get rid of the error by changing /node_modules/polymer-cli/node_modules/polymer-analyzer/lib/javascript/javascript-parser.js plugins array:

...
'classProperties',
'optionalCatchBinding',

Besides the fact that this is a chg in a package not own by myself and is also no longer maintained, the build failed on a different component which does not fail if these two plugins are not added.

Do you see any chance to refactor the static class properties and the code that makes optionalCatchBinding necessary?
Not sure what kind of code the latter is, but I think it's code like this

try {
...
} catch {
}

so a catch w/o an argument.

@FZambia
Copy link
Member Author

FZambia commented Sep 7, 2023

@HJK181 hello, made some adjustments, please check it out again.

@HJK181
Copy link

HJK181 commented Sep 7, 2023

Good morning @FZambia,

tested it with the example application and our real application, it works! Both npm start and npm run build no longer fail with any error. Really nice, thank you very much.

@FZambia FZambia merged commit eb49404 into master Sep 7, 2023
3 checks passed
@FZambia FZambia deleted the esm branch September 7, 2023 07:44
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.

2 participants