Use createRequire with createRequireFromPath as fallback #90
Conversation
ndresx
force-pushed
the
fix-create-require-deprecation-warning
branch
from
609e928
to
a2613d8
Compare
| @@ -240,7 +240,6 @@ export async function createClient( | |||
|
|
|||
| req.on('upgrade', (res, socket, head) => { | |||
| if (res.headers['sec-websocket-accept'] !== digest) { | |||
| res.end(); | |||
There was a problem hiding this comment.
end doesn't exist on this response type (https://nodejs.org/api/http.html#http_class_http_incomingmessage). socket.end() should be enough.
|
Thank you for doing this! I'll review when I get a chance.
Yeah it appears so. It was reported here actions/setup-node#116 but it's happening consistently now. |
| @@ -208,7 +208,7 @@ export function createBridgeFromChildProcess<B extends Bridge>( | |||
| }); | |||
|
|
|||
| proc.on('message', msg => { | |||
| bridge.handleMessage(msg); | |||
| bridge.handleMessage(msg as BridgeMessage); | |||
There was a problem hiding this comment.
Why was this change necessary?
There was a problem hiding this comment.
With the update of @types/node to its latest version, msg is now from type Serializable and not any anymore https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/node/child_process.d.ts#L60.
Argument of type 'Serializable' is not assignable to parameter of type 'BridgeMessage'.
Type 'string' is not assignable to type 'BridgeMessage'
Alternatively, I could change the argument type in handleMessage to make it more transparent, or revert the update of the types module if it's too much action overall for this.
| internalModule.createRequire(filename) || | ||
| internalModule.createRequireFromPath(filename), |
There was a problem hiding this comment.
I think what you meant was
| internalModule.createRequire(filename) || | |
| internalModule.createRequireFromPath(filename), | |
| internalModule.createRequire === undefined ? internalModule.createRequireFromPath(filename) : internalModule.createRequire(filename), | |
There was a problem hiding this comment.
Ups, yes indeed! 😓 I've changed it, thanks for finding this.
|
Looks good to me, thank you! Thanks for updating the definitions too! |
createRequireFromPathis deprecated since Nodev12.2.0(source).According to the contributing documentation, everything above Node
v12is fine, which means it would still need to support the previous implementation ofcreateRequireFromPath.Using
createRequirewas throwing a TS error, which why I have also updated the@types/nodepackage. Therefore, the updated modules are now also part of this PR (separate commit vianpm install), but I'm not sure how this process works here with committing them or not (same goes for the creation of the vendor file). Since it's a separate commit, it could be removed again, or the package update gets reverted (would have to typecreateRequirea bit differently then).Fixes #83
PS: Could it be that there is a problem with the build pipeline step "Run actions/setup-node@v1"? https://github.com/facebookexperimental/rome/pull/90/checks?check_run_id=478564351