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

src: deprecate native typechecking bindings #30818

Open
wants to merge 6 commits into
base: master
from

Conversation

@juanarbol
Copy link
Contributor

juanarbol commented Dec 6, 2019

Deprecate native typechecking bindings at
runtime.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines
@addaleax

This comment has been minimized.

Copy link
Member

addaleax commented Dec 6, 2019

By the way, this is breaking some tests (some = 3 billion).

I assume that’s because we use these methods internally, too, and the added return; breaks the function?

@juanarbol

This comment has been minimized.

Copy link
Contributor Author

juanarbol commented Dec 6, 2019

I guess, also; some tests say that a warning was emitted without expecting it.

@juanarbol

This comment has been minimized.

Copy link
Contributor Author

juanarbol commented Dec 6, 2019

Ah, by the way, I propose this PR because I saw this TODO, I just tried to solve that todo.

@BridgeAR

This comment has been minimized.

Copy link
Member

BridgeAR commented Dec 8, 2019

@juanarbol it should be sufficient to only remove the pendingDeprecation on this line

utilBinding[name] = pendingDeprecation ?
to solve the TODO.

@juanarbol juanarbol force-pushed the juanarbol:deprecate-isX branch from 4e39a10 to eb8cd7e Dec 18, 2019
Copy link
Member

BridgeAR left a comment

The type should be changed in the deprecation documentation to Runtime:

https://github.com/nodejs/node/pull/30818/files#diff-2d795ff9a4ce61626915fd22f1684bb3R2020

lib/internal/bootstrap/pre_execution.js Outdated Show resolved Hide resolved
Copy link
Member

BridgeAR left a comment

LGTM. It would be nice to also add a test case for the deprecation (similar to test/parallel/test-process-env-deprecation.js).

@BridgeAR

This comment has been minimized.

Copy link
Member

BridgeAR commented Jan 2, 2020

@nodejs/tsc PTAL.

@nodejs-github-bot

This comment has been minimized.

Copy link
Member

mcollina left a comment

lgtm

@nodejs-github-bot

This comment has been minimized.

Copy link

nodejs-github-bot commented Jan 3, 2020

Deprecate native typechecking bindings at
runtime.
@juanarbol juanarbol force-pushed the juanarbol:deprecate-isX branch from ceda999 to 7915e35 Jan 3, 2020
@juanarbol

This comment has been minimized.

Copy link
Contributor Author

juanarbol commented Jan 3, 2020

Sorry everyone, I know that when is labeled as author ready you shouldn't make more changes, but test seems to be important here.

Copy link
Member

BridgeAR left a comment

I can't find it right now but if I remember correct, we have some kind of policy that says all files should be lower cased.

I would probably just name the test:
test-native-typechecking-binding-deprecation.js

The comment about the deprecation should also be removed: https://github.com/nodejs/node/pull/30818/files#diff-f15c959d6f6b87e178781150162c835cR244

test/parallel/test-process-isX-deprecation.js Outdated Show resolved Hide resolved
@BridgeAR BridgeAR removed the author ready label Jan 3, 2020
juanarbol added 3 commits Jan 8, 2020
@BridgeAR

This comment has been minimized.

Copy link
Member

BridgeAR commented Jan 12, 2020

@juanarbol would you be so kind and rename the file as suggested in #30818 (review). I should be fine to land this right afterwards.

@juanarbol

This comment has been minimized.

Copy link
Contributor Author

juanarbol commented Jan 12, 2020

@juanarbol would you be so kind and rename the file as suggested in #30818 (review). I should be fine to land this right afterwards.

It's done! Thanks for reviewing.

@nodejs-github-bot

This comment has been minimized.

@Trott

This comment has been minimized.

@Trott
Trott approved these changes Jan 13, 2020
Copy link
Member

Trott left a comment

LGTM although I'd prefer not to land without @addaleax's explicit review/approval since she's the one who wrote the code comment about changing to a runtime deprecation in the first place.

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Jan 13, 2020

Total aside. Feel free to skip....

I can't find it right now but if I remember correct, we have some kind of policy that says all files should be lower cased.

@BridgeAR You might be thinking of doc/STYLE_GUIDE.md but that only refers to documentation files (and, ironically, violates the suggestion itself, although it clearly states there are exceptions without listing them--should probably be updated to say the guidance only applies to API docs, but I'm getting way off topic here....).

FWIW, 52 test files have capital letters in their name at this time:

test/async-hooks/test-crypto-randomBytes.js
test/async-hooks/test-embedder.api.async-resource.runInAsyncScope.js
test/async-hooks/test-fsreqcallback-readFile.js
test/async-hooks/test-graph.fsreq-readFile.js
test/async-hooks/test-timers.setInterval.js
test/async-hooks/test-timers.setTimeout.js
test/known_issues/test-fs-writeFileSync-invalid-windows.js
test/parallel/test-async-hooks-recursive-stack-runInAsyncScope.js
test/parallel/test-cluster-disconnect-exitedAfterDisconnect-race.js
test/parallel/test-cluster-fork-windowsHide.js
test/parallel/test-console-formatTime.js
test/parallel/test-console-not-call-toString.js
test/parallel/test-dgram-createSocket-type.js
test/parallel/test-dgram-multicast-setTTL.js
test/parallel/test-dgram-setBroadcast.js
test/parallel/test-dgram-setTTL.js
test/parallel/test-dns-lookupService.js
test/parallel/test-emit-after-uncaught-exception-runInAsyncScope.js
test/parallel/test-fs-empty-readStream.js
test/parallel/test-fs-makeStatsCallback.js
test/parallel/test-fs-promises-file-handle-readFile.js
test/parallel/test-fs-promises-file-handle-writeFile.js
test/parallel/test-http-contentLength0.js
test/parallel/test-http-createConnection.js
test/parallel/test-http-incoming-matchKnownFields.js
test/parallel/test-http-outgoing-renderHeaders.js
test/parallel/test-http-outgoing-writableFinished.js
test/parallel/test-http2-client-setNextStreamID-errors.js
test/parallel/test-https-client-checkServerIdentity.js
test/parallel/test-internal-util-assertCrypto.js
test/parallel/test-intl-v8BreakIterator.js
test/parallel/test-process-cpuUsage.js
test/parallel/test-querystring-maxKeys-non-finite.js
test/parallel/test-stream-pipe-without-listenerCount.js
test/parallel/test-stream-readable-emittedReadable.js
test/parallel/test-stream-readable-needReadable.js
test/parallel/test-stream-readable-reading-readingMore.js
test/parallel/test-stream-readable-resumeScheduled.js
test/parallel/test-stream-readable-setEncoding-existing-buffers.js
test/parallel/test-stream-readable-setEncoding-null.js
test/parallel/test-stream-readableListening-state.js
test/parallel/test-stream-writableState-ending.js
test/parallel/test-stream-writableState-uncorked-bufferedRequestCount.js
test/parallel/test-timers-clearImmediate.js
test/parallel/test-util-isDeepStrictEqual.js
test/parallel/test-vm-proxy-failure-CP.js
test/parallel/test-zlib-zero-windowBits.js
test/pummel/test-process-cpuUsage.js
test/pummel/test-regress-GH-814.js
test/pummel/test-regress-GH-814_2.js
test/pummel/test-regress-GH-892.js
test/sequential/test-net-GH-5504.js

But lowercase does indeed seem to be the norm.

@addaleax

This comment has been minimized.

Copy link
Member

addaleax commented Jan 13, 2020

@Trott Yeah, I regret writing that comment – I’m not a fan of doing this, tbh. There’s no need, and --pending-deprecation seems like exactly the right level of deprecation for this kind of thing (an aliased API).

@nodejs-github-bot

This comment has been minimized.

@tniessen tniessen dismissed their stale review Jan 16, 2020

The documentation says that this has been Superseded by DEP0111, which won't be true if we make this a runtime deprecation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.