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

Remove local fork of acorn JS parser #11439

Closed
sbc100 opened this issue Jun 18, 2020 · 6 comments · Fixed by #11480
Closed

Remove local fork of acorn JS parser #11439

sbc100 opened this issue Jun 18, 2020 · 6 comments · Fixed by #11480

Comments

@sbc100
Copy link
Collaborator

sbc100 commented Jun 18, 2020

We currently have a local fork of acorn in-tree, but it would be more elegant to simply install it via npm.

Apparently there is a local patch that upstream don't want but a suggested workaround exists #11435

@ghost
Copy link

ghost commented Jun 23, 2020

Hello, I think you wanna delete acornjs code in third_party directory and add to package.json.
Is it correct?

@sbc100
Copy link
Collaborator Author

sbc100 commented Jun 23, 2020

Yes, but also we have a local patch which contains functionality which we would like to preserve. I believe @RReverser has suggested some way to get the desired behaviour without the local patch.

Thanks for the offer of help!

@ghost
Copy link

ghost commented Jun 23, 2020

Can I ask a question?
First, I saw the history of built file of acorn(https://github.com/emscripten-core/emscripten/commits/master/third_party/acorn/dist/acorn.js)
But there is no changes.
So I saw the PR and I guess @kripken wanna add some error print. (acornjs/acorn#793)
If I write about this issue, I'll wrap this line(https://github.com/emscripten-core/emscripten/blob/master/tools/acorn-optimizer.js#L1302) with try~catch.
But now there is no code in this repository.
Where is the local patch?
Am I misunderstood about this issue or miss something..? 🤔

@RReverser
Copy link
Collaborator

Yeah it's quite hard to find the source change of Acorn, but it appears the built file was modified here

// XXX EMSCRIPTEN: add a quote of the error text, point to where
// https://github.com/acornjs/acorn/pull/793
.

FWIW my suggestion was the following: as you can see, the errors raised by Acorn have pos and loc properties, so if someone wanted to, they could simply try-catch the error and use those properties to produce a message in an arbitrary format.

@TroyTae I think the easiest might be to just update Acorn code to latest upstream in Emscripten repo, and then look at the expected format vs retrieved one when running tests and wrap accordingly.

@kripken
Copy link
Member

kripken commented Jun 23, 2020

It's also possible we can just replace acorn and all tests would pass - I think we may have removed an overly-strict test for the error format. Worth opening a PR with just that, and seeing test results.

@ghost
Copy link

ghost commented Jun 24, 2020

@RReverser Thank you! It seems history is lost after move directory 😅
As you said, error object has loc and pos, so maybe I can handle this.

@kripken Now I'm trying to do it. If we need some strict test, I'll write it :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants