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

assert,deps,repl: fix repl and assert not being able to handle language features properly #27400

Closed
wants to merge 13 commits into from

Conversation

Projects
None yet
6 participants
@BridgeAR
Copy link
Member

commented Apr 24, 2019

This adds a acorn plugins to properly handle stage-3 language features in the REPL and assert.

I stripped the plugins to the minimum and also removed a couple of files that we will not use to reduce our code overhead in core.

I had to change the require calls in the plugins to work properly with core but I guess that's expected.

The numeric separators are not yet supported by V8 but it should be supported by v7.5, so adding it now makes this feature available from the moment we include the new version.

There are a few more plugins for other language features: acorn-dynamic-import, acorn-import-meta and acorn-export-ns-from. These are all @nodejs/modules related and I am not sure if we should include these as well or not. I felt these are not suitable for us but I might be wrong?

We currently include the main acorn license file which states Copyright (C) 2012-2018 by various contributors (see AUTHORS). The plugin licenses are also MIT licenses but it states Copyright (C) 2017-2018 by Adrian Heine. I am not sure if the various contributors would be sufficient here, since this is a single person for these modules even though they are hosted in the acornjs organization.
If we have to pull in the other licenses, should I move the plugins out into another folder so that we use a single license for all plugins together (they are all written by the same person / have the same license) or should we include all licenses individually?

Fixes: #27391
Fixes: #25835

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

BridgeAR added some commits Apr 24, 2019

deps: add acorn stage-3 plugins
This adds bigint, class-fields, numeric-separators, static-class
features, private class methods and fields as dependency. That way
it's possible to use these in combination with acorn to parse these
language features.

This also removes a couple of files that were not necessary for
Node.js to reduce the code base.
assert: use new language features
This adds new language features to acorn. Otherwise BigInt and other
input would not be parsed correct and would not result in nice error
messages when using simple assert.
repl: add new language features to top level await statements
This adds stage-3 language features to acorn so that it's possible
to parse these features when using top level await in the REPL.
repl: handle stage-3 language features properly
This adds stage-3 language features to acorn so that the REPL is
able to parse these features properly. Otherwise these would cause
SyntaxErrors.
@nodejs-github-bot

This comment has been minimized.

@GeoffreyBooth

This comment has been minimized.

Copy link
Contributor

commented Apr 24, 2019

@BridgeAR Take a look at nodejs/ecmascript-modules#69. I was working around the lack of import() support in Acorn for that PR, so having such support would be nice (but not required).

@BridgeAR

This comment has been minimized.

Copy link
Member Author

commented Apr 24, 2019

@GeoffreyBooth I suggest that you add the necessary Plugin to your PR but I am not sure if it's necessary for assert or the REPL to have support for this.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@BridgeAR BridgeAR requested review from targos, mcollina and Fishrock123 Apr 25, 2019

@BridgeAR

This comment has been minimized.

Copy link
Member Author

commented Apr 26, 2019

@nodejs/tsc PTAL about the licensing questions (see #27400 (comment)).

@nodejs/assert @nodejs/repl PTAL

@targos

This comment has been minimized.

Copy link
Member

commented Apr 26, 2019

My suggestion regarding the license:

  • Move all acorn plugins to a new directory: deps/acorn-plugins
  • targos@6d8e444
Show resolved Hide resolved lib/assert.js Outdated

BridgeAR added some commits Apr 26, 2019

@BridgeAR

This comment has been minimized.

Copy link
Member Author

commented Apr 26, 2019

@targos I incorporated your suggestion.

@nodejs-github-bot

This comment has been minimized.

@targos

This comment has been minimized.

Copy link
Member

commented Apr 26, 2019

Can you run tools/license-builder.sh and commit the updated license file?

@targos

This comment was marked as resolved.

Copy link
Member

commented Apr 26, 2019

Error in REPL tests:

19:13:33 not ok 1539 parallel/test-repl
19:13:33   ---
19:13:33   duration_ms: 0.62
19:13:33   severity: fail
19:13:33   exitcode: 1
19:13:33   stack: |-
19:13:33     internal/bootstrap/loaders.js:189
19:13:33       return mod.compile();
19:13:33                  ^
19:13:33     
19:13:33     TypeError: Cannot read property 'compile' of undefined
19:13:33         at nativeModuleRequire (internal/bootstrap/loaders.js:189:14)
19:13:33         at requireWithFallbackInDeps (internal/bootstrap/loaders.js:207:10)
19:13:33         at internal/deps/acorn-plugins/acorn-private-methods/index.js:3:30
19:13:33         at NativeModule.compile (internal/bootstrap/loaders.js:300:5)
19:13:33         at nativeModuleRequire (internal/bootstrap/loaders.js:189:14)
19:13:33         at internal/repl/utils.js:5:3
19:13:33         at NativeModule.compile (internal/bootstrap/loaders.js:300:5)
19:13:33         at nativeModuleRequire (internal/bootstrap/loaders.js:189:14)
19:13:33         at repl.js:84:5
19:13:33         at NativeModule.compile (internal/bootstrap/loaders.js:300:5)
@BridgeAR

This comment has been minimized.

Copy link
Member Author

commented Apr 29, 2019

This could use some reviews. It fixes language features not being available in the REPL and it would be good to move this forward.

@addaleax
Copy link
Member

left a comment

LGTM

@targos

targos approved these changes Apr 30, 2019

BridgeAR added a commit to BridgeAR/node that referenced this pull request Apr 30, 2019

deps: add acorn stage-3 plugins
This adds bigint, class-fields, numeric-separators, static-class
features, private class methods and fields as dependency. That way
it's possible to use these in combination with acorn to parse these
language features.

This also removes a couple of files that were not necessary for
Node.js to reduce the code base.

PR-URL: nodejs#27400
Refs: nodejs#27391
Refs: nodejs#25835
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>

BridgeAR added a commit to BridgeAR/node that referenced this pull request Apr 30, 2019

assert: use new language features
This adds new language features to acorn. Otherwise BigInt and other
input would not be parsed correct and would not result in nice error
messages when using simple assert.

PR-URL: nodejs#27400
Refs: nodejs#27391
Refs: nodejs#25835
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>

BridgeAR added a commit to BridgeAR/node that referenced this pull request Apr 30, 2019

repl: add new language features to top level await statements
This adds stage-3 language features to acorn so that it's possible
to parse these features when using top level await in the REPL.

PR-URL: nodejs#27400
Refs: nodejs#27391
Refs: nodejs#25835
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>

BridgeAR added a commit to BridgeAR/node that referenced this pull request Apr 30, 2019

repl: handle stage-3 language features properly
This adds stage-3 language features to acorn so that the REPL is
able to parse these features properly. Otherwise these would cause
SyntaxErrors.

PR-URL: nodejs#27400
Fixes: nodejs#27391
Fixes: nodejs#25835
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>

BridgeAR added a commit to BridgeAR/node that referenced this pull request Apr 30, 2019

test: add tests for new language features
PR-URL: nodejs#27400
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@BridgeAR

This comment has been minimized.

Copy link
Member Author

commented Apr 30, 2019

Thanks for the reviews.

Landed in 98e9de7...3d2409c 🎉

@BridgeAR BridgeAR closed this Apr 30, 2019

targos added a commit that referenced this pull request Apr 30, 2019

deps: add acorn stage-3 plugins
This adds bigint, class-fields, numeric-separators, static-class
features, private class methods and fields as dependency. That way
it's possible to use these in combination with acorn to parse these
language features.

This also removes a couple of files that were not necessary for
Node.js to reduce the code base.

PR-URL: #27400
Refs: #27391
Refs: #25835
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>

targos added a commit that referenced this pull request Apr 30, 2019

assert: use new language features
This adds new language features to acorn. Otherwise BigInt and other
input would not be parsed correct and would not result in nice error
messages when using simple assert.

PR-URL: #27400
Refs: #27391
Refs: #25835
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>

targos added a commit that referenced this pull request Apr 30, 2019

repl: add new language features to top level await statements
This adds stage-3 language features to acorn so that it's possible
to parse these features when using top level await in the REPL.

PR-URL: #27400
Refs: #27391
Refs: #25835
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>

targos added a commit that referenced this pull request Apr 30, 2019

repl: handle stage-3 language features properly
This adds stage-3 language features to acorn so that the REPL is
able to parse these features properly. Otherwise these would cause
SyntaxErrors.

PR-URL: #27400
Fixes: #27391
Fixes: #25835
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>

targos added a commit that referenced this pull request Apr 30, 2019

test: add tests for new language features
PR-URL: #27400
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>

@targos targos referenced this pull request May 6, 2019

Merged

v12.2.0 proposal #27578

targos added a commit that referenced this pull request May 7, 2019

2019-05-07, Version 12.2.0 (Current)
Notable changes:

* deps:
  * Updated llhttp to 1.1.3. This fixes a bug that made Node.js' HTTP
    parser refuse any request URL that contained the "|" (vertical bar)
    character. #27595
* tls:
  * Added an `enableTrace()` method to `TLSSocket` and an `enableTrace`
    option to `tls.createServer()`. When enabled, TSL packet trace
    information is written to `stderr`. This can be used to debug TLS
    connection problems. #27497
    #27376
* cli:
  * Added a `--trace-tls` command-line flag that enables tracing of TLS
    connections without the need to modify existing application code.
    #27497
  * Added a `--cpu-prof-interval` command-line flag. It can be used to
    specify the sampling interval for the CPU profiles generated by
    `--cpu-prof`. #27535
* module:
  * Added the `createRequire()` method. It allows to create a require
    function from a file URL object, a file URL string or an absolute
    path string. The existing `createRequireFromPath()` method is now
    deprecated #27405.
  * Throw on `require('./path.mjs')`. This is technically a breaking
    change that should have landed with Node.js 12.0.0. It is necessary
    to have this to keep the possibility for a future minor version to
    load ES Modules with the require function.
    #27417
* repl:
  * The REPL now supports multi-line statements using `BigInt` literals
    as well as public and private class fields and methods.
    #27400
  * The REPL now supports tab autocompletion of file paths with `fs`
    methods. #26648
* meta:
  * Added Christian Clauss (https://github.com/cclauss) to
    collaborators. #27554

PR-URL: #27578

targos added a commit that referenced this pull request May 7, 2019

2019-05-07, Version 12.2.0 (Current)
Notable changes:

* deps:
  * Updated llhttp to 1.1.3. This fixes a bug that made Node.js' HTTP
    parser refuse any request URL that contained the "|" (vertical bar)
    character. #27595
* tls:
  * Added an `enableTrace()` method to `TLSSocket` and an `enableTrace`
    option to `tls.createServer()`. When enabled, TSL packet trace
    information is written to `stderr`. This can be used to debug TLS
    connection problems. #27497
    #27376
* cli:
  * Added a `--trace-tls` command-line flag that enables tracing of TLS
    connections without the need to modify existing application code.
    #27497
  * Added a `--cpu-prof-interval` command-line flag. It can be used to
    specify the sampling interval for the CPU profiles generated by
    `--cpu-prof`. #27535
* module:
  * Added the `createRequire()` method. It allows to create a require
    function from a file URL object, a file URL string or an absolute
    path string. The existing `createRequireFromPath()` method is now
    deprecated #27405.
  * Throw on `require('./path.mjs')`. This is technically a breaking
    change that should have landed with Node.js 12.0.0. It is necessary
    to have this to keep the possibility for a future minor version to
    load ES Modules with the require function.
    #27417
* repl:
  * The REPL now supports multi-line statements using `BigInt` literals
    as well as public and private class fields and methods.
    #27400
  * The REPL now supports tab autocompletion of file paths with `fs`
    methods. #26648
* meta:
  * Added Christian Clauss (https://github.com/cclauss) to
    collaborators. #27554

PR-URL: #27578
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.