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.

Sign up for GitHub

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

Jump to bottom

lib: ensure --check flag works with --require #19600

Closed
wants to merge 1 commit into from
Closed

lib: ensure --check flag works with --require #19600

wants to merge 1 commit into from

Conversation

jdalton
Copy link
Member

@jdalton jdalton commented Mar 25, 2018 •
edited
Loading

--eval, --print, and --check-with-piped-input work with --require.
However, --check with a file path does not. This PR fixes #18425.

Checklist

@jdalton jdalton mentioned this pull request Mar 25, 2018
Closed
@addaleax addaleax added semver-major PRs that contain breaking changes and should be released in the next major version. cli Issues and PRs related to the Node.js command line interface. labels Mar 25, 2018
@addaleax
Copy link
Member

I'd prefer to be careful and tag this semver-major... I can't tell whether this patch makes sense in the use cases for -c, but as I understand it, this would now make Node.js run userland code with an option where that was never the case previously.

(This should probably also be documented?)

@jdalton
Copy link
Member Author

jdalton commented Mar 25, 2018 •
edited
Loading

I'd prefer to be careful and tag this semver-major...

semver-major sounds fine to me.

I can't tell whether this patch makes sense in the use cases for -c,

This covers the @babel/register, and other runtime instrumenters, case by fixing the gap in existing functionality -- as mentioned --require works for --check today, but only for piped input.

(This should probably also be documented?)

Cool.

Ya, the current docs for flags like --check is a little light.
For example, there is no mention that --check and --eval cannot be used together
(though there is a runtime warning).

Would docs be done in this PR or a follow-up?

@jdalton
Copy link
Member Author

jdalton commented Mar 28, 2018

Ping @addaleax.

@addaleax
Copy link
Member

Would docs be done in this PR or a follow-up?

I don’t think you should have to expand the documentation with what you think should be in there in this PR, but this particular change should probably be documented as a change under both options by adding a changes: block, something like this. How does that sound?

@jdalton
Copy link
Member Author

jdalton commented Mar 28, 2018 •
edited
Loading

How does that sound?

Sounds good. I'll update the PR today!

Update:

Added change note.

@jdalton
Copy link
Member Author

jdalton commented Mar 30, 2018 •
edited
Loading

@addaleax or @jasnell could one of you trigger a CI run (just to confirm tests are good to go)

@jdalton
Copy link
Member Author

jdalton commented Apr 1, 2018 •
edited
Loading

Ping @addaleax or @jasnell.

@richardlau
Copy link
Member

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

richardlau
richardlau reviewed Apr 1, 2018
View reviewed changes
test/parallel/test-cli-syntax.js Outdated
['-c', '--check'].forEach(function(checkFlag) {
['-r', '--require'].forEach(function(requireFlag) {
const preloadFile = fixtures.path('no-wrapper.js');
const file = fixtures.path('syntax/illegal_if_not_wrapped.js');
Copy link
Member

Choose a reason for hiding this comment

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

fixtures.path('syntax', 'illegal_if_not_wrapped.js')

Copy link
Member Author

@jdalton jdalton Apr 1, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@richardlau Updated. You might do a follow-up PR to tweak the rest of the file's usage if that is important to you.

@jdalton
Copy link
Member Author

jdalton commented Apr 1, 2018

@richardlau Do node-test-commit-linux and node-test-commit-smartos fails look legit?

@jdalton
Copy link
Member Author

jdalton commented Apr 2, 2018

Ping @richardlau.

@jdalton
Copy link
Member Author

jdalton commented Apr 4, 2018

Can someone please rerun the CI check run or confirm that the fails are not related.

@jasnell
Copy link
Member

jasnell commented Apr 4, 2018

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

@jdalton
Copy link
Member Author

jdalton commented Apr 4, 2018 •
edited
Loading

Thanks @jasnell!

Now it looks like a different set of unrelated tests are failing

node-test-binary-arm
40 - parallel/test-cluster-master-kill
298 - sequential/test-net-GH-5504

Update:

@jasnell assuming the tests are false fails and this lands. Would it make it into Node 10?

@jasnell
Copy link
Member

jasnell commented Apr 5, 2018

Assuming it lands by next Tuesday (which it should).

@jdalton
Copy link
Member Author

jdalton commented Apr 5, 2018 •
edited
Loading

Cool. Do you think these fails are real. I'm wondering if there's anything actionable for me to do on my side of things or if it's just a matter of hitting re-test until the other flaky tests pass.

@jasnell
Copy link
Member

jasnell commented Apr 5, 2018

The failures are unrelated, this should be able to land.

jasnell pushed a commit that referenced this pull request Apr 5, 2018
@jdalton @jasnell
lib: ensure --check flag works with --require
0876a03 
PR-URL: #19600
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
@jasnell
Copy link
Member

jasnell commented Apr 5, 2018

Landed in 0876a03

@jasnell jasnell closed this Apr 5, 2018
@jdalton jdalton deleted the check branch April 5, 2018 00:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richardlau richardlau richardlau approved these changes

@addaleax addaleax addaleax approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
cli Issues and PRs related to the Node.js command line interface. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

--check work with --require
4 participants
@jdalton @addaleax @richardlau @jasnell