Skip to content

assert: lazy load acorn #19863

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

Closed
wants to merge 1 commit into from
Closed

Conversation

BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Apr 7, 2018

This makes sure acorn is only loaded in case it is necessary. This prevents acorn always being loaded on startup.

I decided to use the modules cache instead of special handling this. The loading
is only done rarely.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

This makes sure acorn is only loaded in case it is necessary.
@BridgeAR BridgeAR requested a review from joyeecheung April 7, 2018 12:41
@nodejs-github-bot nodejs-github-bot added the assert Issues and PRs related to the assert subsystem. label Apr 7, 2018
@BridgeAR
Copy link
Member Author

BridgeAR commented Apr 7, 2018

@BridgeAR BridgeAR added fast-track PRs that do not need to wait for 48 hours to land. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Apr 7, 2018
@ChALkeR
Copy link
Member

ChALkeR commented Apr 7, 2018

Doesn't this make --trace-sync-io nag? This could be a semver-major.
Also, I am surprized to learn that assert now requires acorn (which is pretty huge).

Upd: ah, acorn was landed in #17581 which is a semver-major and didn't get into 9.x, so probably no need to label this semver-major.

I still am not very comfortable on having assert depend on acorn (and nag on --trace-sync-io) though.

@BridgeAR
Copy link
Member Author

BridgeAR commented Apr 9, 2018

@ChALkeR this change will only load acorn once, when it is first required. After that, the module cache should be hit besides the error cache that is implemented here as well.

I am not a huge fan of using acorn either but it makes simple assert much more powerful. If there is a different way of doing this: I would love to know.

@BridgeAR
Copy link
Member Author

BridgeAR commented Apr 9, 2018

@ChALkeR I guess your comment is not blocking this PR, right?

BridgeAR added a commit to BridgeAR/node that referenced this pull request Apr 9, 2018
This makes sure acorn is only loaded in case it is necessary.

PR-URL: nodejs#19863
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BridgeAR
Copy link
Member Author

BridgeAR commented Apr 9, 2018

Landed in 9c06770

Since the author-ready label was not removed, I guessed it was fine to land this.

@BridgeAR BridgeAR closed this Apr 9, 2018
@ChALkeR
Copy link
Member

ChALkeR commented Apr 10, 2018

@BridgeAR My comment was not blocking this, but I am unhappy with how this upsets --trace-sync-io. Not sure how to work-around that while keeping acorn, though. It would be much better if V8 exposed its js parsing API, like SpiderMonkey does.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assert Issues and PRs related to the assert subsystem. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. fast-track PRs that do not need to wait for 48 hours to land.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants