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
base: master
from

Conversation

Projects
None yet
6 participants
@jasnell
Member

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 changed the title from Use emitwarning to lib,fs: use process.emitWarning instead of internal print dep warning Aug 18, 2016

@ChALkeR

This comment has been minimized.

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

This comment has been minimized.

Member

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

This comment has been minimized.

Member

vkurchatkin commented Aug 18, 2016

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

@ChALkeR

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

Member

jasnell commented Aug 18, 2016

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

@ChALkeR

This comment has been minimized.

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

This comment has been minimized.

Member

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

This comment has been minimized.

Member

vkurchatkin commented Aug 18, 2016

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

@jasnell

This comment has been minimized.

Member

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

This comment has been minimized.

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 jasnell force-pushed the jasnell:use-emitwarning branch Aug 18, 2016

@jasnell

This comment has been minimized.

Member

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)
@ChALkeR

View changes

lib/fs.js Outdated
@@ -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.

This comment has been minimized.

@ChALkeR

ChALkeR Aug 18, 2016

Member

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.

This comment has been minimized.

@jasnell

jasnell Aug 18, 2016

Member

Yeah, will add that back

@ChALkeR

This comment has been minimized.

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

This comment has been minimized.

Member

jasnell commented Aug 18, 2016

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

@ChALkeR

View changes

lib/fs.js Outdated
@@ -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

This comment has been minimized.

@ChALkeR

ChALkeR Aug 18, 2016

Member

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

This comment has been minimized.

Member

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

This comment has been minimized.

Member

jasnell commented Aug 21, 2016

@nodejs/ctc ... PTAL

@jasnell

This comment has been minimized.

Member

jasnell commented Aug 23, 2016

Ping @nodejs/ctc @nodejs/collaborators

@ChALkeR

View changes

lib/fs.js Outdated
// 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.

This comment has been minimized.

@ChALkeR

ChALkeR Aug 25, 2016

Member

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

This comment has been minimized.

Member

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

This comment has been minimized.

Member

jasnell commented Aug 25, 2016

CI is green

@jasnell jasnell referenced this pull request Aug 25, 2016

Closed

Revert "fs: add a temporary fix for re-evaluation support" #6413

2 of 2 tasks complete

@jasnell jasnell force-pushed the jasnell:use-emitwarning branch Aug 30, 2016

@jasnell

This comment has been minimized.

Member

jasnell commented Aug 30, 2016

@ChALkeR ... PR updated, PTAL

@jasnell

This comment has been minimized.

@MylesBorins

This comment has been minimized.

Member

MylesBorins commented Aug 30, 2016

LGTM

@jasnell

This comment has been minimized.

Member

jasnell commented Aug 30, 2016

Flaky failure in arm, CI is green otherwise.

@jasnell

This comment has been minimized.

Member

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;

This comment has been minimized.

@ChALkeR

ChALkeR Aug 30, 2016

Member

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.

This comment has been minimized.

@jasnell

jasnell Sep 1, 2016

Member

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

This comment has been minimized.

@ChALkeR

ChALkeR Sep 1, 2016

Member

Only if those land before this one =).

This comment has been minimized.

@jasnell

jasnell Sep 2, 2016

Member

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

This comment has been minimized.

@ChALkeR

ChALkeR Sep 2, 2016

Member

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

jasnell added some commits Aug 18, 2016

lib: use emitWarning instead of printDeprecationMessage
The process.emitWarning() API should be used for printing
deprecation warning messages rather than directly using the
internal/util#printDeprecationMessage
fs: use process.emitWarning to print deprecation warning
As an alternative to #6413, use
process.emitWarning() instead of the internal printDeprecationMessage
in order to avoid use of an internal only API.
internal/util: remove printDeprecationWarning
Removes the internal/util printDeprecationWarning method

@jasnell jasnell force-pushed the jasnell:use-emitwarning branch to 2ef2c7f Sep 2, 2016

@jasnell

This comment has been minimized.

Member

jasnell commented Sep 2, 2016

@jasnell

This comment has been minimized.

Member

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

process: fix handling of process.noDeprecation in emitWarning
PR-URL: #8166
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>

jasnell added a commit that referenced this pull request Sep 2, 2016

lib: use emitWarning instead of printDeprecationMessage
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: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>

jasnell added a commit that referenced this pull request Sep 2, 2016

fs: use process.emitWarning to print deprecation warning
Use process.emitWarning() instead of the internal printDeprecationMessage
in order to avoid use of an internal only API.

PR-URL: #8166
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>

jasnell added a commit that referenced this pull request Sep 2, 2016

internal/util: remove printDeprecationWarning
Removes the internal/util printDeprecationWarning method

PR-URL: #8166
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
@jasnell

This comment has been minimized.

Member

jasnell commented Sep 2, 2016

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

@jasnell

This comment has been minimized.

Member

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

process: fix handling of process.noDeprecation in emitWarning
Cherry-picked from:
  PR-URL: nodejs#8166
  Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
  Reviewed-By: Myles Borins <myles.borins@gmail.com>

@gibfahn gibfahn referenced this pull request Jun 15, 2017

Closed

Auditing for 6.11.1 #230

2 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment