FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

assert: lazy load acorn by BridgeAR · Pull Request #19863 · nodejs/node · GitHub

/ node Public

assert: lazy load acorn - #19863

Closed
BridgeAR wants to merge 1 commit into
nodejs:masterfrom
BridgeAR:lazy-load-acorn
ConversationCommits (1)ChecksFiles changed
Closed

assert: lazy load acorn#19863
BridgeAR wants to merge 1 commit into
nodejs:masterfrom
BridgeAR:lazy-load-acorn

Conversation

BridgeAR commented Apr 7, 2018

Copy link
Copy Markdown
Member

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

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

BridgeAR commented Apr 7, 2018

Copy link
Copy Markdown
Member Author

CI https://ci.nodejs.org/job/node-test-pull-request/14118/

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 outstanding review comments, and a CI started. labels Apr 7, 2018

ChALkeR commented Apr 7, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

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 commented Apr 9, 2018

Copy link
Copy Markdown
Member Author

@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 commented Apr 9, 2018

Copy link
Copy Markdown
Member Author

@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
assert: lazy load acorn
9c06770 
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 commented Apr 9, 2018

Copy link
Copy Markdown
Member Author

Landed in 9c06770

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

BridgeAR closed this Apr 9, 2018

ChALkeR commented Apr 10, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

@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.

joyeecheung mentioned this pull request Apr 14, 2018
Closed
BridgeAR deleted the lazy-load-acorn branch April 1, 2019 23:40
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell approved these changes
lpinca lpinca approved these changes
joyeecheung joyeecheung approved these changes

Assignees

No one assigned

Labels

assert Issues and PRs related to the assert subsystem. author ready PRs that have at least one approval, no outstanding review comments, and a CI started. fast-track PRs that do not need to wait for 48 hours to land.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

Back | FazBrowse Home | New Git URL