WIP: feat(node): v13#40438
WIP: feat(node): v13#40438SimonSchick wants to merge 2 commits intoDefinitelyTyped:masterfrom SimonSchick:feat/node-v13
Conversation
|
@SimonSchick Thank you for submitting this PR! Because this is a new definition, a DefinitelyTyped maintainer will be reviewing this PR in the next few days once the Travis CI build passes. In the meantime, if the build fails or a merge conflict occurs, I'll let you know. Have a nice day! |
|
@SimonSchick The Travis CI build failed! Please review the logs for more information. Once you've pushed the fixes, the build will automatically re-run. Thanks! |
|
@SimonSchick The Travis CI build failed! Please review the logs for more information. Once you've pushed the fixes, the build will automatically re-run. Thanks! |
|
@SimonSchick The Travis CI build failed! Please review the logs for more information. Once you've pushed the fixes, the build will automatically re-run. Thanks! |
|
@SimonSchick The Travis CI build failed! Please review the logs for more information. Once you've pushed the fixes, the build will automatically re-run. Thanks! |
|
Hey @Flarna mind giving some early input? |
|
@SimonSchick The Travis CI build failed! Please review the logs for more information. Once you've pushed the fixes, the build will automatically re-run. Thanks! |
|
Had a few minutes only therefore not seen most of the changes.
|
|
@SimonSchick The Travis CI build failed! Please review the logs for more information. Once you've pushed the fixes, the build will automatically re-run. Thanks! |
| prependOnceListener(event: "setup", listener: (settings: ClusterSettings) => void): this; | ||
| } | ||
|
|
||
| const SCHED_NONE: number; |
There was a problem hiding this comment.
should we add these two also to interface cluster?
There was a problem hiding this comment.
Trying to dedupe cluster, will take care of this tho!
| } | ||
|
|
||
| { | ||
| new events(); |
There was a problem hiding this comment.
actually this should work, see https://github.com/nodejs/node/blob/64eacd8741f23cb91569faf7ec793c83badb04e3/lib/events.js#L46
There was a problem hiding this comment.
Interesting, have you seen this used anywhere in the ecosystem?
Things like these are super annoying to model and usually require hacks such as fake inheritance.
Any ideas?
There was a problem hiding this comment.
Can't remember a specific place; I expect this is a relict from node 0.x so if this is used somewhere then most likely in old modules.
It's the same/similar for module (see https://github.com/nodejs/node/blob/v13.1.0/lib/internal/modules/cjs/loader.js#L1261) and stream as also similar.
What was the problem with the exisiting variant export = internal?
There was a problem hiding this comment.
I figured v13 was a good opportunity to clean things up a bit and remove this virtual inheritance nonsense, but it sounds like this could break a few people 🤔
There was a problem hiding this comment.
I'm actually pretty curious to see how many people would be affected by this.
Do you think it would be an acceptable risk to do this on v13 or does it have too much potential for breaking changes?
There was a problem hiding this comment.
As far as I know ES6 modules don't allow such exports anyway so seems to be a step forward. Fixing an app is easy so I'm ok with it. Reverting in case of a lot complains should be also not that hard.
There was a problem hiding this comment.
Same applies to module, stream. Although I won't be touching stream for now.
There was a problem hiding this comment.
Honestly speaking I would be also careful with module as this is usually just required to get the static methods exposed there. So at least these should be kept.
| // Buffer class | ||
| type BufferEncoding = "ascii" | "utf8" | "utf-8" | "utf16le" | "ucs2" | "ucs-2" | "base64" | "latin1" | "binary" | "hex"; | ||
|
|
||
| interface Buffer { |
There was a problem hiding this comment.
It's not really needed and was a special case that was added just because someone wanted to have access to the type safe constructor ¯\(ツ)/¯
There was a problem hiding this comment.
I think I remember... was related to webpack to avoid that too much is packed.
So you plan is to break the build for this user? 😕
There was a problem hiding this comment.
That sounds odd, usually types don't affect webpack builds.
There was a problem hiding this comment.
#28900 there is no mention of Webpack in this issue. Seems like it was pure convenience, since buffer was moved to a class it most likely doesn't matter anymore.
There was a problem hiding this comment.
The comment was Because if i do that, bundlers include an entire Buffer polyfill, I assumed the bundler was webpack but could be anything.
Anyhow, I feel not well in removing this added by someone else by a dedicated PR. It doesn't harm and let's hope that typescript is at some time able to ship this out of the box.
There was a problem hiding this comment.
webpack doesn't implicitly emit node polyfills anymore.
And in any case the person can just cast Buffer.constructor to typeof Buffer 😛
types/node/module.d.ts
Outdated
| * @deprecated Deprecated since: v12.2.0. Please use createRequire() instead. | ||
| */ | ||
| static createRequireFromPath(path: string): NodeJS.RequireFunction; | ||
| static createRequire(path: string): NodeJS.RequireFunction; |
There was a problem hiding this comment.
A while ago I tried also to move Module from gobal to here but failed for some reason and never created a PR. Maybe because of some typescript version no longer supported...
Anyhow, could you please add URL support here: static createRequire(path: string | URL) and import { URL } from "url";.
That time I had also some you could reuse now:
import m = require('module');
import { createRequireFromPath, createRequire } from 'module';
import { URL } from "url";
let rf: (m: string) => any;
rf = createRequire('mod');
rf = createRequireFromPath('mod');
rf = createRequire(new URL('file:///C:/path/'));
const aModule: NodeModule = new m.Module("s");
const bModule: NodeModule = new m("b", aModule);
const builtIn: string[] = m.builtinModules;
|
@sandersn I'm not sure who's in an authority position but can we potentially halt all merged to the node package until v13 is done? Is there any way we can move forward in getting this in? |
|
@SimonSchick The Travis CI build failed! Please review the logs for more information. Once you've pushed the fixes, the build will automatically re-run. Thanks! |
|
Great, so the requirement for file inclusion changed... |
|
You haven't touched tsconfig, so you should be able to merge from master to get rid of the now-excess d.ts references in the files list. See microsoft/types-publisher#708 for details. |
|
As for reviewers from the TS team, @rbuckton is the local node expert, although he's on vacation for Thanksgiving until next Monday. |
|
I missed updating the moved tsconfig file yes, I already fixed it. |
|
Now there's a failure with an unused test file: Unused file /home/vsts/work/1/s/types/node/v12/test/async_hooks.ts (used files: ["assert.ts","buffer.ts","child_process.ts","cluster.ts","crypto.ts","dgram.ts","events.ts","fs.ts","global.ts","http.ts","http2.ts","net.ts","os.ts","path.ts","perf_hooks.ts","process.ts","querystring.ts","readline.ts","repl.ts","stream.ts","string_decoder.ts","tls.ts","tty.ts","util.ts","v8.ts","vm.ts","worker_threads.ts","zlib.ts"]) This looks to me like it's just missing from v12/tsconfig.json. |
|
Yea I'm on it, I tried to be too smart with git :) |
|
Seems like there are more packages that violate the new tsconfig rules: Btw it would be great if it would emit a list of all files, not just the first it can find and bail. |
|
Is that from a local run? I don't see it on my local run. You merged from master already, right? Edit: And I don't see it on the Azure run either. |
|
Well no, but apparently npm doesn't like updating sometimes when using non npm packages eg. |
|
@SimonSchick The Travis CI build failed! Please review the logs for more information. Once you've pushed the fixes, the build will automatically re-run. Thanks! |
|
@SimonSchick I haven't seen anything from you in a while and this PR currently has problems that prevent it from being merged. The PR will be closed tomorrow if there aren't new commits to fix the issues. |
|
@rbuckton Can we move forward on this before the bot it's thing again (mildly frustrating btw). |
|
@SimonSchick To keep things tidy, we have to close PRs that aren't mergeable but don't have activity from their author. No worries, though - please open a new PR if you'd like to continue with this change. Thank you! |
|
I'm growing progressively dissatisfied with this bot... |
|
@rbuckton (sorry for the many mentions), but what do? |
|
ping |
|
@SimonSchick To keep a PR alive it's needed to keep the commit date young. a merge from master, rebase or just |
|
I'm hesitant to create a new PR after seeing how this one went. |
Please fill in this template.
npm test.)npm run lint package-name(ortscif notslint.jsonis present).Select one of these and delete the others:
If changing an existing definition:
tslint.jsoncontaining{ "extends": "dtslint/dt.json" }. If for reason the any rule need to be disabled, disable it for that line using// tslint:disable-next-line [ruleName]and not for whole package so that the need for disabling can be reviewed.This ended up being quite a large PR and I want some feedback on if we can proceed with this or if this needs to be broken down further.
General list of changes:
constantsmodule to avoid duplication.Have to potentially revert sinceasync iteration ones retained///references don't supportlibuntil TS 3.xMovedvarious global interface intoNodeJSnamespace.Have to potentially retain some globals to not break other packages.clusterunique symbolEventEmitterclass with interface as it's purely virtual and only there for global declarations.extends EventEmitterorextends NodeJS.EventEmitter.This is somewhat WIP and I expect lots of changes.