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 for class methods named "static" #15

Merged
merged 4 commits into from
May 29, 2020

Conversation

kumavis
Copy link
Contributor

@kumavis kumavis commented May 29, 2020

status: ready 🎉

Fixes #13
Fixes #14

encountered this parsing fast-glob via mdeps using resolve

@goto-bus-stop @ljharb

@kumavis
Copy link
Contributor Author

kumavis commented May 29, 2020

build script is still broken (see #14 )
I patched the build script locally, generated the updated files, and then only included the relevant update for the sake of a minimal change. Other updates are ready in-range.

However, this patch seems to have introduced a regression with class private fields? not sure why

@kumavis
Copy link
Contributor Author

kumavis commented May 29, 2020

can reproduce the regression by switching between the two parser extensions here
the build script is breaking something somehow

const acorn = require("acorn")
const Parser = acorn.Parser.extend(require("acorn-static-class-features"))
// const Parser = acorn.Parser.extend(require("./lib/static-class-features"))

const result = Parser.parse("class X { static #x = y }", { ecmaVersion: 10 })

@kumavis
Copy link
Contributor Author

kumavis commented May 29, 2020

narrowed it down to acorn-private-class-elements's transpile, though don't understand the issue yet 🤔

@kumavis
Copy link
Contributor Author

kumavis commented May 29, 2020

oof ok, solved it
unsurprisingly, the quick-and-dirty transform in the build script is brittle

@kumavis
Copy link
Contributor Author

kumavis commented May 29, 2020

here is an issue for buble relating to us needing to do the manual fix at all bublejs/buble#259

@kumavis
Copy link
Contributor Author

kumavis commented May 29, 2020

btw there's in range updates for the following, and the dynamic import thing should be figured out, but out of scope for this issue

	modified:   lib/class-fields/index.js
	modified:   lib/import-meta/index.js
	modified:   lib/numeric-separator/index.js

@goto-bus-stop
Copy link
Member

We should def switch to Babel, I liked Bublé's smallness but AFAIK it's not actively maintained now and we're already having to pile up hacks to make the build work 😅

Copy link
Member

@goto-bus-stop goto-bus-stop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gonna do the babel switch separately. Thanks for the PR!

@goto-bus-stop goto-bus-stop merged commit b88000f into browserify:master May 29, 2020
@kumavis kumavis deleted the static-fix branch May 29, 2020 15:54
@kumavis
Copy link
Contributor Author

kumavis commented May 29, 2020

@goto-bus-stop thanks/ would really appreciate a patch version today if possible 🙏

@kumavis
Copy link
Contributor Author

kumavis commented May 30, 2020

@goto-bus-stop sorry to bother, i would really appreciate if you could publish a patch ✨

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.

npm build fails Bug: doesn't support class functions named "static"
2 participants