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

lib,fs: use process.emitWarning instead of internal print dep warning #8166

Closed
wants to merge 4 commits into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Aug 18, 2016

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

lib, fs

Description of change

This removes uses of the the internal/util printDeprecationMessage() API in favor of
using process.emitWarning(msg, 'DeprecationWarning'). This allows us to remove the
graceful-fs warning in fs providing at least a temporary relief of the logjam that is
occurring there.

The PR also adds a new lint rule that flags uses of printDeprecationMessage()

/cc @ChALkeR @thealphanerd @nodejs/ctc

This is an alternative to #6413. If this lands, We do not have to do the revert.

@jasnell jasnell added fs Issues and PRs related to the fs subsystem / file system. lib / src Issues and PRs related to general changes in the lib or src directory. labels Aug 18, 2016
@nodejs-github-bot nodejs-github-bot added util Issues and PRs related to the built-in util module. module Issues and PRs related to the module subsystem. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. labels Aug 18, 2016
@jasnell jasnell changed the title Use emitwarning lib,fs: use process.emitWarning instead of internal print dep warning Aug 18, 2016
@ChALkeR
Copy link
Member

ChALkeR commented Aug 18, 2016

Sorry, but -1 on that until #6413 gets merged. We should keep the warning there until it's broken, else we will need to re-implement the warning.

Because other PRs (I counted four of them) are blocked by the exact same thing — and we don't want those to be broken without a warning.

That warning is not a culpit, it's just the messenger, just removing it for the sake of removing would do no good, if other PRs will block on that again.

After #6413 and other breaking PRs get merged, +1 on this change as a regular clean up, but not as an alternative to #6413.

@jasnell
Copy link
Member Author

jasnell commented Aug 18, 2016

I understand that the same fundamental problem exists still but this would eliminate the immediate issue giving us more time to address the larger concern without jamming anything up on this one thing. It's an incremental step not a final solution.

@vkurchatkin
Copy link
Contributor

I don't understand, why it is removes from all modules, not just fs?

@ChALkeR
Copy link
Member

ChALkeR commented Aug 18, 2016

@jasnell Once again: evading the breaking change in #6413 would not give us anything, because there are several more PRs that break graceful-fs evildoing in exactly the same way. That's why the fix in graceful-fs@3 (isaacs/node-graceful-fs@07701dc) also wouldn't work generally.

Moreover — #6413 doesn't even break graceful-fs anymore, as the hack was landed in graceful-fs (which I am not happy about, because it gives us nothing).

The issue here is with other PRs which get blocked by keeping graceful-fs@3 compatibility, and that's what we need to fix. Removing the deprecation warning would make things even worse, I think. We still address the issue that blocks those PR as «#6413», but that's not even correct anymore — it's only a label for the issue atm as #6413 could be merged at any time with no effect on graceful-fs@3.

@ChALkeR
Copy link
Member

ChALkeR commented Aug 18, 2016

@jasnell I would be completely fine with merging this as a regular clean up if the deprecation warning is kept and if we will have a separate PR that would turn the deprecation warning into a throw instead of #6413, so that we could block all those PRs against that instead of #6413.

-1 is only for the removal of the warning without making it a throw.

For the record, a list of PRs why we need the warning: #6749, #6573, #7162, #2025 (that last one is closed, but it got closed because of graceful-fs hack support, it actually does a good thing).

Any of those would break graceful-fs@[23], and we don't want that to be a sudden breakage. The warning is required so that people stop using graceful-fs@[23] so we could merge those.

@jasnell
Copy link
Member Author

jasnell commented Aug 18, 2016

@ChALkeR ... just so I understand... are you specifically talking about the graceful-fs warning?

@ChALkeR
Copy link
Member

ChALkeR commented Aug 18, 2016

@jasnell I am talking about the fs: re-evaluating native module sources is not supported. … warning that is currently triggered on the Node.js side when someone tries to re-evaluate core fs moule sources (namely, graceful-fs@2 and graceful-fs@3).

@jasnell
Copy link
Member Author

jasnell commented Aug 18, 2016

@vkurchatkin ... if you look at the implementation of internal/util#printDeprecationMessage, you'll see that all it does is defer to process.emitWarning(). Accordingly, it doesn't make sense to keep using printDeprecationMessage since we can use emitWarning() directly.

@vkurchatkin
Copy link
Contributor

but why create eslint rule for that? let's just remove printDeprecationMessage

@jasnell
Copy link
Member Author

jasnell commented Aug 18, 2016

We can do that also. My plan was to reevaluate the methods in internal/util separately and remove stuff that's unnecessary in a separate PR, but you're right that this would allow us to go ahead and remove printDeprecationMessage() now.

@Trott
Copy link
Member

Trott commented Aug 18, 2016

Nit: I would prefer a clearer name for the rule and rule file. dep is more likely to be read as "dependency" or something like that at first glance and not deprecation. Maybe no-internal-dep -> no-internal-deprecation at a minimum? I'm fine with the rule being long. There's already longer built-in rule names like no-whitespace-before-property.

@jasnell
Copy link
Member Author

jasnell commented Aug 18, 2016

@vkurchatkin @ChALkeR .. ok, refactored the PR.

  • The internal/util printDeprecationMessage() method is removed entirely
  • The reevaluation warning is added back into fs but without the need for polyfilling printDeprecationMessage()
  • I had to include a fix to emitWarning because it wasn't handling process.noDeprecation correctly (bug that wasn't caught previously)

@@ -15,6 +15,18 @@ const EventEmitter = require('events');
const FSReqWrap = binding.FSReqWrap;
const FSEvent = process.binding('fs_event_wrap').FSEvent;

// Check for re-evaluation of the module. Doing so will cause the internal
// module to fail to load.
Copy link
Member

@ChALkeR ChALkeR Aug 18, 2016

Choose a reason for hiding this comment

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

Could you keep the TODO and reasoning? It might need a slight rephrase, e.g.:

// TODO(ChALkeR): remove this in master after 6.x
// This is required to give users a grace period before actually breaking
// modules that re-evaluate fs sources from context where internal
// modules are not allowed, e.g. older versions of graceful-fs module.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, will add that back

@ChALkeR
Copy link
Member

ChALkeR commented Aug 18, 2016

@jasnell Thanks!

+1 to the current approach. This is not a complete review, I will review the actual code changes tomorrow =).

@jasnell
Copy link
Member Author

jasnell commented Aug 18, 2016

@Trott ... I just removed the eslint rule but thank you for the feedback on it :-)

@@ -15,8 +15,11 @@ const EventEmitter = require('events');
const FSReqWrap = binding.FSReqWrap;
const FSEvent = process.binding('fs_event_wrap').FSEvent;

// TODO(ChALkeR,jasnell): remove the following try/catch in master after 7.x
Copy link
Member

Choose a reason for hiding this comment

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

I'm still hoping we could remove the warning and land other PRs in time to v7.0 release, as the ecosystem packages actually could get fixed in time to v7.0 release.

Atm, this comment contradicts our milestones, specifically, milestone for #6573 — if that lands in v7.0, then this warning would be useless in v7.0.

@jasnell
Copy link
Member Author

jasnell commented Aug 18, 2016

@ChALkeR I know.. this reflects the current status I think but if we can clear up the logjam before v7 then fantastic...

@jasnell
Copy link
Member Author

jasnell commented Aug 21, 2016

@nodejs/ctc ... PTAL

@jasnell
Copy link
Member Author

jasnell commented Aug 23, 2016

Ping @nodejs/ctc @nodejs/collaborators

// Check for re-evaluation of the module. Doing so will cause the internal
// module to fail to load. This is required to give users a grace period before
// actually breaking modules that re-evaluate fs sources from context where
// internal modules are not allowed, e.g. older versions of graceful-fs module.
Copy link
Member

@ChALkeR ChALkeR Aug 25, 2016

Choose a reason for hiding this comment

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

Note that this is still valid (even with the most recent graceful-fs@3 update) for users of graceful-fs@2, older versions of graceful-fs@3, and perhaps some user code that does the same thing (unlikely, but still possible).

Removing the warning would force us to introduce it again at least for a full major cycle in order to land other PRs that are blocked by re-evaluating fs.

So, please don't remove this on updates =).

@jasnell
Copy link
Member Author

jasnell commented Aug 25, 2016

New CI: https://ci.nodejs.org/job/node-test-pull-request/3832/
(the last one uncovered a bug.. updated)

@jasnell
Copy link
Member Author

jasnell commented Aug 25, 2016

CI is green

@jasnell
Copy link
Member Author

jasnell commented Aug 30, 2016

@ChALkeR ... PR updated, PTAL

@jasnell
Copy link
Member Author

jasnell commented Aug 30, 2016

@MylesBorins
Copy link
Contributor

LGTM

@jasnell
Copy link
Member Author

jasnell commented Aug 30, 2016

Flaky failure in arm, CI is green otherwise.

@jasnell
Copy link
Member Author

jasnell commented Aug 30, 2016

Will land this tomorrow if there are no objections.

@@ -40,7 +40,6 @@ const isWindows = process.platform === 'win32';

const DEBUG = process.env.NODE_DEBUG && /fs/.test(process.env.NODE_DEBUG);
const errnoException = util._errnoException;
const printDeprecation = require('internal/util').printDeprecationMessage;
Copy link
Member

@ChALkeR ChALkeR Aug 30, 2016

Choose a reason for hiding this comment

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

Could you leave require('internal/util'); here so this won't become suddenly re-evaluatable in v7? Or we might be forced to do another round of deprecation…

That line could be removed when something else will land that requires internal, hopefully in time for v7.

Upd: not needed anymore, as #6749 landed.

Copy link
Member Author

Choose a reason for hiding this comment

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

There are several other open PRs that move bits to internal/fs.js that would cover this requirement.

Copy link
Member

Choose a reason for hiding this comment

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

Only if those land before this one =).

Copy link
Member Author

Choose a reason for hiding this comment

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

@ChALkeR ... #6749 landed just now, which adds a ref to internal/fs

Copy link
Member

Choose a reason for hiding this comment

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

Yay!
That discards my previous comments in this subthread =).

The process.emitWarning() API should be used for printing
deprecation warning messages rather than directly using the
internal/util#printDeprecationMessage
As an alternative to nodejs#6413, use
process.emitWarning() instead of the internal printDeprecationMessage
in order to avoid use of an internal only API.
Removes the internal/util printDeprecationWarning method
@jasnell
Copy link
Member Author

jasnell commented Sep 2, 2016

New CI before landing: https://ci.nodejs.org/job/node-test-pull-request/3930/

@jasnell
Copy link
Member Author

jasnell commented Sep 2, 2016

CI is green-ish. Only build bot failures that will hopefully be fixed soon. Landing!

jasnell added a commit that referenced this pull request Sep 2, 2016
PR-URL: #8166
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
jasnell added a commit that referenced this pull request Sep 2, 2016
The process.emitWarning() API should be used for printing
deprecation warning messages rather than directly using the
internal/util#printDeprecationMessage

PR-URL: #8166
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
jasnell added a commit that referenced this pull request Sep 2, 2016
Use process.emitWarning() instead of the internal printDeprecationMessage
in order to avoid use of an internal only API.

PR-URL: #8166
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
jasnell added a commit that referenced this pull request Sep 2, 2016
Removes the internal/util printDeprecationWarning method

PR-URL: #8166
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
@jasnell
Copy link
Member Author

jasnell commented Sep 2, 2016

Landed in bf91035, 15eaba9, b50557b, and 7b73f55

@jasnell
Copy link
Member Author

jasnell commented Sep 2, 2016

Marking as don't land on v6 or v4 due to the potential legacy graceful-fs impact.

jasnell added a commit to jasnell/node that referenced this pull request Sep 2, 2016
Cherry-picked from:
  PR-URL: nodejs#8166
  Reviewed-By: Сковорода Никита Андреевич <[email protected]>
  Reviewed-By: Myles Borins <[email protected]>
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fs Issues and PRs related to the fs subsystem / file system. lib / src Issues and PRs related to general changes in the lib or src directory. module Issues and PRs related to the module subsystem. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants