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

[v10.x] fs: stop lazy loading stream constructors by targos · Pull Request #21776 · nodejs/node · GitHub

/ node Public

[v10.x] fs: stop lazy loading stream constructors - #21776

Closed
targos wants to merge 1 commit into
nodejs:v10.x-stagingfrom
targos:fix-fs-mock
ConversationCommits (1)ChecksFiles changed
Closed

[v10.x] fs: stop lazy loading stream constructors#21776
targos wants to merge 1 commit into
nodejs:v10.x-stagingfrom
targos:fix-fs-mock

Conversation

targos commented Jul 12, 2018

Copy link
Copy Markdown
Member

Fixes: #21489

/cc @nodejs/fs @SimenB

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

targos added the fs Issues and PRs related to the fs subsystem / file system. label Jul 12, 2018

nodejs-github-bot commented Jul 12, 2018

Copy link
Copy Markdown
Collaborator

@targos build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/255/pipeline

nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Jul 12, 2018
joyeecheung approved these changes Jul 12, 2018
View reviewed changes

joyeecheung left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Do we need to add a test to make sure these public members of fs are mockable (i.e. if you assign fs.member to something else then assert.strictEqual(fs.member, mockedMember)? It should be less useful for other modules but members of fs are mocked by users all the time in tests.

joyeecheung reviewed Jul 12, 2018
View reviewed changes
Comment thread lib/fs.js Outdated

joyeecheung Jul 12, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

An alternative fix is adding setters here to allow users to override the lazy-loaded variables so we can still avoid loading the internal/fs/streams module when requiring fs.

targos Jul 12, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

That's how it's implemented now. It doesn't work with jest-mock because accessors are ignored.

joyeecheung Jul 12, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Ah right, I see that there are setters...so this looks more like a bug of Jest?

targos Jul 12, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Yes, maybe. But this was still kind of a sever major change and I'm not sure it's worth to do it.

joyeecheung commented Jul 12, 2018

Copy link
Copy Markdown
Member

Should we just wait until Jest fixes the bug instead of working around it ourselves? Looks like Jest just need to consider properties with setters as properties that can be overridden instead of bypassing every property with a getter in the descriptor?

joyeecheung requested changes Jul 12, 2018
View reviewed changes

joyeecheung left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Putting a red cross because it does not look like it has to be us fixing the regression, since we already provide setters (also because GitHub does not allow you to dismiss approvals..). This can be dismissed if it's shown that it has to be us fixing this.

vitkarpov reviewed Jul 12, 2018
View reviewed changes
Comment thread lib/internal/fs/streams.js Outdated

vitkarpov Jul 12, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Why not to just return require('fs') since require already has a cache?

jasnell Jul 12, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

The require cache is slightly more expensive so we generally use this pattern through core to keep it at cheap as possible. It's likely unnecessary in most cases, but it's a consistent pattern.

vitkarpov Jul 12, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Okay, cool. Thank you!

BridgeAR Jul 12, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@jasnell we might want to benchmark the influence of that again. It would actually be so clean if we'd just use require directly.

SimenB commented Jul 12, 2018

Copy link
Copy Markdown
Member

I'm fine with fixing this in Jest (although I won't have time for it anytime soon, leaving for vacation), but it has over 1.3 million every week, and people are generally slow to update. It also broke in a minor, not a major (and the original PR was originally marked as major before passing CITGM).

That said, nobody has complained to us about it not working, only noticed since our own tests broke 🙂

BridgeAR commented Jul 12, 2018

Copy link
Copy Markdown
Member

@joyeecheung I personally would say it was a unintentionally breaking change. It was the reason why I did not move that part out when I was working on improving lazy loading in Node core.

So I am +1 on merging this at least on 10.

jasnell commented Jul 12, 2018

Copy link
Copy Markdown
Member

I'm good with this change in v10 but keeping the current behavior in master and 11.

jasnell commented Jul 12, 2018

Copy link
Copy Markdown
Member

Specifically, what I'd like to see is this PR rebased against v10.x-staging, rather than against master.

targos changed the base branch from master to v10.x-staging July 14, 2018 10:43
targos changed the title fs: stop lazy loading stream constructors [v10.x] fs: stop lazy loading stream constructors Jul 14, 2018
targos added the v10.x label Jul 14, 2018

targos commented Jul 14, 2018

Copy link
Copy Markdown
Member Author

Updated to target v10.x-staging. PTAL

targos commented Jul 17, 2018

Copy link
Copy Markdown
Member Author

@joyeecheung are you fine with this landing on v10.x only?

targos commented Jul 18, 2018 •
edited
Loading

Copy link
Copy Markdown
Member Author

CI: https://ci.nodejs.org/job/node-test-pull-request/15922/
Edit: resumed build: https://ci.nodejs.org/job/node-test-pull-request/15941/

targos commented Jul 19, 2018 •
edited
Loading

Copy link
Copy Markdown
Member Author

Landed in 484140e

targos closed this Jul 19, 2018
targos added a commit that referenced this pull request Jul 19, 2018
fs: stop lazy loading stream constructors
484140e 
Fixes: #21489

PR-URL: #21776
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
targos deleted the fix-fs-mock branch July 19, 2018 09:09
targos mentioned this pull request Jul 31, 2018
Merged
This was referenced Aug 1, 2018
Closed
Closed
This was referenced Aug 1, 2018
Closed
Closed
Closed
Closed
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 left review comments
cjihrig cjihrig approved these changes
joyeecheung joyeecheung approved these changes
BridgeAR BridgeAR approved these changes
+1 more reviewer
vitkarpov vitkarpov left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

fs Issues and PRs related to the fs subsystem / file system.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

Back | FazBrowse Home | New Git URL