GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — build: runtime-deprecate requiring deps by TimothyGu · Pull Request #16392 · nodejs/node

Conversation

@TimothyGu

@TimothyGu TimothyGu commented Oct 22, 2017

Copy link
Copy Markdown
Member

Fixes: #15566 (comment)

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
Affected core subsystem(s)

build

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to Node.js builds or CI infrastructure. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. tools Issues and PRs related to the tools directory. labels Oct 22, 2017
@TimothyGu
TimothyGu force-pushed the deps-modules branch from aa42362 to 7ea4135 Compare October 22, 2017 21:58
@TimothyGu TimothyGu mentioned this pull request Oct 22, 2017
repl: support top-level await #15566
Closed
3 of 4 tasks
@TimothyGu
TimothyGu requested a review from bnoordhuis October 22, 2017 22:02
refack
refack approved these changes Oct 22, 2017
View reviewed changes
@refack

refack commented Oct 22, 2017

Copy link
Copy Markdown
Contributor

Isn't this major
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bnoordhuis
bnoordhuis reviewed Oct 23, 2017
View reviewed changes

@bnoordhuis bnoordhuis left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

FWIW, I'd be okay with simple removal. It seems vanishingly unlikely that anyone is using these.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread tools/js2c.py Outdated

@bnoordhuis bnoordhuis Oct 23, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Style nit: deprecated_deps

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@TimothyGu TimothyGu Nov 10, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Fixed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread tools/js2c.py Outdated

@bnoordhuis bnoordhuis Oct 23, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This maps deps/foo/bar.js to internal/deps/foo/bar.js.

I don't think you have to special-case for node-inspect or v8 because anything from deps/ should pretty much be internal by default. ISTM it can be simplified to his:

if split[0] === 'deps':
  split[0] = 'internal'
else:
  split = split[1:]

Could be simplified further by just matching on name:

name = '/'.join(re.split('/|\\\\', name))
if name.startswith('lib/'):
  name = name[len('lib/'):]
elif name.startswith('deps/'):
  deprecated_deps, name = name, 'internal/' + name[len('deps/'):]

Bonus points: you won't have to '/'.join(...) below.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@bnoordhuis bnoordhuis Oct 23, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I think I misread the intent of this code - it's prefixing with internal/ unconditionally but stashing the original away. Could still be simplified, though:

name = '/'.join(re.split('/|\\\\', name))
if name.startswith('deps/node-inspect/') or name.startswith('deps/v8/'):
  deprecated_deps = name[len('deps/'):]
  name = 'internal/' + name
elif name.startswith('lib/'):
  name = name[len('lib/'):]

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@TimothyGu TimothyGu Nov 10, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

@bnoordhuis Not only that, but internal/ should also be added to any other file in deps/ that do not match node-inspect and v8 to prevent them from being visible to userland, thus the code in its current form.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
jasnell
jasnell approved these changes Oct 23, 2017
View reviewed changes

@jasnell jasnell left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM once @bnoordhuis is happy also.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@jasnell jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Oct 23, 2017
cjihrig
cjihrig approved these changes Nov 1, 2017
View reviewed changes

@cjihrig cjihrig left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM once nits are addressed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
jasnell
jasnell approved these changes Nov 2, 2017
View reviewed changes

@jasnell jasnell left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

generally LGTM once @bnoordhuis is happy also

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@TimothyGu
build: runtime-deprecate requiring deps
122176c 
Fixes: nodejs#15566 (comment)
@TimothyGu
TimothyGu force-pushed the deps-modules branch from 7ea4135 to 122176c Compare November 10, 2017 21:46
@TimothyGu

TimothyGu commented Nov 10, 2017

Copy link
Copy Markdown
Member Author

@bnoordhuis PTAL.

New CI: https://ci.nodejs.org/job/node-test-pull-request/11345/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bnoordhuis
bnoordhuis approved these changes Nov 13, 2017
View reviewed changes
@TimothyGu

TimothyGu commented Nov 13, 2017

Copy link
Copy Markdown
Member Author

Landed in 0e10717.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@TimothyGu TimothyGu closed this Nov 13, 2017
@TimothyGu
TimothyGu deleted the deps-modules branch November 13, 2017 18:51
TimothyGu added a commit that referenced this pull request Nov 13, 2017
@TimothyGu
build: runtime-deprecate requiring deps
0e10717 
PR-URL: #16392
Fixes: #15566 (comment)
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
@targos targos mentioned this pull request Nov 29, 2017
module: add builtinModules #16386
Closed
2 of 3 tasks
@cjihrig cjihrig mentioned this pull request Jan 10, 2018
[v9.x backport] test: fix require-deps-deprecation for installed deps #18077
Closed
3 tasks done
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bnoordhuis bnoordhuis bnoordhuis approved these changes
@jasnell jasnell jasnell approved these changes
@cjihrig cjihrig cjihrig approved these changes
+1 more reviewer
@refack refack refack approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

build Issues and PRs related to Node.js builds or CI infrastructure. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. semver-major PRs that contain breaking changes and should be released in the next major version. tools Issues and PRs related to the tools directory.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@TimothyGu @refack @bnoordhuis @jasnell @cjihrig @nodejs-github-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.