-
Notifications
You must be signed in to change notification settings - Fork 29k
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: replace MessageEvent with undici's #52370
Conversation
Review requested:
|
undici's MessageEvent is better tested and has a complete WebIDL implementation for validation. Not only this, but it's also used in Node's current WebSocket implementation. There are a large number of webidl-related issues in the current MessageEvent, such as not implementing `MessageEvent.prototype.initMessageEvent`, not validating arguments passed to its constructor (nodejs#51771), not validating the values passed to the constructor (such as not validating that `ports` is a sequence, not converting origin to a USVString, etc.), and other issues. fixup
b4ed037
to
f126982
Compare
Have you considered the other way around? Fixing it in Node.js and reusing it in undici? Is it harder to port tests here? |
It is significantly harder to implement here without a real webidl implementation. |
}); | ||
assert.throws(() => { | ||
new MessageEvent('message', { source: {} }); | ||
}, { | ||
code: 'ERR_INVALID_ARG_TYPE', | ||
message: /The "init\.source" property must be an instance of MessagePort/, | ||
name: 'TypeError', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we are changing error code this becomes a semver-major
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@KhafraDev it would be better if this PR wouldn't be semver-major, so we can land it in v22.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recall @addaleax did a lot of work optimizing Node's message event for workers, so pleasae run the MessageEvent benchmarks before landing this.
MessageEvent should be compatible with the spec if it's marked as a compliant version of MessageEvent. |
I'd like to move this forward if possible, especially now that WebSocket is a global in node. I have no intention on supporting Node's current "MessageEvent" in WebSocket so it seems like we're in a standstill here. I would be willing to expose a utility function to create an undici MessageEvent that skips webidl validations, which should yield the same performance as it is currently. Is that an acceptable solution here? Inevitably, as it is right now, undici cannot beat the current MessageEvent's performance, because node's MessageEvent does nothing correct, spec-wise. |
@benjamingr I experimented with this and I was able to make instantiating an undici MessageEvent over 3x faster than the current node one. https://github.com/nodejs/undici/compare/main...KhafraDev:undici:fast-messageevent?expand=1
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
The only concern I have is that undici can be not always available, therefore blocking something else entirely. |
I pinged @addaleax 3 weeks ago, that seems plenty of time to respond and worst case we can always revisit things. |
For future reference @KhafraDev our process allows people to dismiss requests for feedback/changes after 7 days of unresponsiveness. I wanted Anna to be able to say her piece but that's not an excuse for me blocking a PR and neither was it my intention. |
You brought up a valid point and it'll lead to faster MessageEvent creation in node core and undici. I don't think that outright dismissing a valid review without further discussion would be a good idea, especially since I am not familiar with node workers. |
@KhafraDev absolutely, feedback is important - that's why we wait a week (or more if people ask nicely and explain why) :) In this case it was not my intention to block the change just to try and get feedback. I recall there is a good reason for the current design but it has been around 4-5 years since and we have tests and benchmarks. Anyway, just wanted to apologize and to make sure you know in the future you're free to dismiss these sort of reviews after a week of no response. |
@KhafraDev can you make this non-semver-major? |
I'll try |
I don't think it's viable. Errors are all handled uniformly in undici's webidl, so if we change the message in one place every other message will similarly get changed. It makes for much better, more consistent, errors, but doesn't give us much room to use custom messages. I did add error codes for brand checks and I am gradually improving the existing errors, but I don't think we'll be able to match the current ones. |
Landed in 3136fb0 |
undici's MessageEvent is better tested and has a complete WebIDL implementation for validation. Not only this, but it's also used in Node's current WebSocket implementation. There are a large number of webidl-related issues in the current MessageEvent, such as not implementing `MessageEvent.prototype.initMessageEvent`, not validating arguments passed to its constructor (#51771), not validating the values passed to the constructor (such as not validating that `ports` is a sequence, not converting origin to a USVString, etc.), and other issues. fixup PR-URL: #52370 Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com>
undici's MessageEvent is better tested and has a complete WebIDL implementation for validation. Not only this, but it's also used in Node's current WebSocket implementation. There are a large number of webidl-related issues in the current MessageEvent, such as not implementing `MessageEvent.prototype.initMessageEvent`, not validating arguments passed to its constructor (nodejs#51771), not validating the values passed to the constructor (such as not validating that `ports` is a sequence, not converting origin to a USVString, etc.), and other issues. fixup PR-URL: nodejs#52370 Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com>
undici's MessageEvent is better tested and has a complete WebIDL implementation for validation. Not only this, but it's also used in Node's current WebSocket implementation. There are a large number of webidl-related issues in the current MessageEvent, such as not implementing `MessageEvent.prototype.initMessageEvent`, not validating arguments passed to its constructor (#51771), not validating the values passed to the constructor (such as not validating that `ports` is a sequence, not converting origin to a USVString, etc.), and other issues. fixup PR-URL: #52370 Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com>
Notable changes: buffer: * (SEMVER-MINOR) add .bytes() method to Blob (Matthew Aitken) #53221 cli: * (SEMVER-MINOR) add `NODE_RUN_PACKAGE_JSON_PATH` env (Yagiz Nizipli) #53058 * (SEMVER-MINOR) add `NODE_RUN_SCRIPT_NAME` env to `node --run` (Yagiz Nizipli) #53032 doc: * (SEMVER-MINOR) add context.assert docs (Colin Ihrig) #53169 * (SEMVER-MINOR) improve explanation about built-in modules (Joyee Cheung) #52762 * add StefanStojanovic to collaborators (StefanStojanovic) #53118 fs: * mark recursive cp methods as stable (Théo LUDWIG) #53127 lib: * (SEMVER-MINOR) add EventSource Client (Aras Abbasi) #51575 * (SEMVER-MINOR) replace MessageEvent with undici's (Matthew Aitken) #52370 module: * (SEMVER-MINOR) print amount of load time of a cjs module (Vinicius Lourenço) #52213 net: * (SEMVER-MINOR) add new net.server.listen tracing channel (Paolo Insogna) #53136 process: * (SEMVER-MINOR) add process.getBuiltinModule(id) (Joyee Cheung) #52762 src,permission: * (SEMVER-MINOR) --allow-wasi & prevent WASI exec (Rafael Gonzaga) #53124 test_runner: * (SEMVER-MINOR) add snapshot testing (Colin Ihrig) #53169 * (SEMVER-MINOR) add context.fullName (Colin Ihrig) #53169 * (SEMVER-MINOR) support module mocking (Colin Ihrig) #52848 PR-URL: TODO
Notable changes: buffer: * (SEMVER-MINOR) add .bytes() method to Blob (Matthew Aitken) #53221 cli: * (SEMVER-MINOR) add `NODE_RUN_PACKAGE_JSON_PATH` env (Yagiz Nizipli) #53058 * (SEMVER-MINOR) add `NODE_RUN_SCRIPT_NAME` env to `node --run` (Yagiz Nizipli) #53032 doc: * (SEMVER-MINOR) add context.assert docs (Colin Ihrig) #53169 * (SEMVER-MINOR) improve explanation about built-in modules (Joyee Cheung) #52762 * add StefanStojanovic to collaborators (StefanStojanovic) #53118 * add Marco Ippolito to TSC (Rafael Gonzaga) #53008 fs: * mark recursive cp methods as stable (Théo LUDWIG) #53127 lib: * (SEMVER-MINOR) add EventSource Client (Aras Abbasi) #51575 * (SEMVER-MINOR) replace MessageEvent with undici's (Matthew Aitken) #52370 module: * (SEMVER-MINOR) print amount of load time of a cjs module (Vinicius Lourenço) #52213 net: * (SEMVER-MINOR) add new net.server.listen tracing channel (Paolo Insogna) #53136 process: * (SEMVER-MINOR) add process.getBuiltinModule(id) (Joyee Cheung) #52762 src,permission: * (SEMVER-MINOR) --allow-wasi & prevent WASI exec (Rafael Gonzaga) #53124 test_runner: * (SEMVER-MINOR) add snapshot testing (Colin Ihrig) #53169 * (SEMVER-MINOR) add context.fullName (Colin Ihrig) #53169 * (SEMVER-MINOR) support module mocking (Colin Ihrig) #52848 PR-URL: #53379
Notable changes: buffer: * (SEMVER-MINOR) add .bytes() method to Blob (Matthew Aitken) #53221 cli: * (SEMVER-MINOR) add `NODE_RUN_PACKAGE_JSON_PATH` env (Yagiz Nizipli) #53058 * (SEMVER-MINOR) add `NODE_RUN_SCRIPT_NAME` env to `node --run` (Yagiz Nizipli) #53032 doc: * (SEMVER-MINOR) add context.assert docs (Colin Ihrig) #53169 * (SEMVER-MINOR) improve explanation about built-in modules (Joyee Cheung) #52762 * add StefanStojanovic to collaborators (StefanStojanovic) #53118 * add Marco Ippolito to TSC (Rafael Gonzaga) #53008 fs: * mark recursive cp methods as stable (Théo LUDWIG) #53127 lib: * (SEMVER-MINOR) add EventSource Client (Aras Abbasi) #51575 * (SEMVER-MINOR) replace MessageEvent with undici's (Matthew Aitken) #52370 module: * (SEMVER-MINOR) print amount of load time of a cjs module (Vinicius Lourenço) #52213 net: * (SEMVER-MINOR) add new net.server.listen tracing channel (Paolo Insogna) #53136 process: * (SEMVER-MINOR) add process.getBuiltinModule(id) (Joyee Cheung) #52762 src: * (SEMVER-MINOR) traverse parent folders while running `--run` (Yagiz Nizipli) #53154 src,permission: * (SEMVER-MINOR) --allow-wasi & prevent WASI exec (Rafael Gonzaga) #53124 test_runner: * (SEMVER-MINOR) add snapshot testing (Colin Ihrig) #53169 * (SEMVER-MINOR) add context.fullName (Colin Ihrig) #53169 * (SEMVER-MINOR) support module mocking (Colin Ihrig) #52848 PR-URL: #53379
Notable changes: buffer: * (SEMVER-MINOR) add .bytes() method to Blob (Matthew Aitken) #53221 cli: * (SEMVER-MINOR) add `NODE_RUN_PACKAGE_JSON_PATH` env (Yagiz Nizipli) #53058 * (SEMVER-MINOR) add `NODE_RUN_SCRIPT_NAME` env to `node --run` (Yagiz Nizipli) #53032 doc: * (SEMVER-MINOR) add context.assert docs (Colin Ihrig) #53169 * (SEMVER-MINOR) improve explanation about built-in modules (Joyee Cheung) #52762 * add StefanStojanovic to collaborators (StefanStojanovic) #53118 * add Marco Ippolito to TSC (Rafael Gonzaga) #53008 fs: * mark recursive cp methods as stable (Théo LUDWIG) #53127 lib: * (SEMVER-MINOR) add EventSource Client (Aras Abbasi) #51575 * (SEMVER-MINOR) replace MessageEvent with undici's (Matthew Aitken) #52370 module: * (SEMVER-MINOR) print amount of load time of a cjs module (Vinicius Lourenço) #52213 net: * (SEMVER-MINOR) add new net.server.listen tracing channel (Paolo Insogna) #53136 process: * (SEMVER-MINOR) add process.getBuiltinModule(id) (Joyee Cheung) #52762 src: * (SEMVER-MINOR) traverse parent folders while running `--run` (Yagiz Nizipli) #53154 src,permission: * (SEMVER-MINOR) --allow-wasi & prevent WASI exec (Rafael Gonzaga) #53124 test_runner: * (SEMVER-MINOR) add snapshot testing (Colin Ihrig) #53169 * (SEMVER-MINOR) add context.fullName (Colin Ihrig) #53169 * (SEMVER-MINOR) support module mocking (Colin Ihrig) #52848 PR-URL: #53379
Notable changes: buffer: * (SEMVER-MINOR) add .bytes() method to Blob (Matthew Aitken) #53221 cli: * (SEMVER-MINOR) add `NODE_RUN_PACKAGE_JSON_PATH` env (Yagiz Nizipli) #53058 * (SEMVER-MINOR) add `NODE_RUN_SCRIPT_NAME` env to `node --run` (Yagiz Nizipli) #53032 doc: * (SEMVER-MINOR) add context.assert docs (Colin Ihrig) #53169 * (SEMVER-MINOR) improve explanation about built-in modules (Joyee Cheung) #52762 * add StefanStojanovic to collaborators (StefanStojanovic) #53118 * add Marco Ippolito to TSC (Rafael Gonzaga) #53008 fs: * mark recursive cp methods as stable (Théo LUDWIG) #53127 lib: * (SEMVER-MINOR) add EventSource Client (Aras Abbasi) #51575 * (SEMVER-MINOR) replace MessageEvent with undici's (Matthew Aitken) #52370 module: * (SEMVER-MINOR) print amount of load time of a cjs module (Vinicius Lourenço) #52213 net: * (SEMVER-MINOR) add new net.server.listen tracing channel (Paolo Insogna) #53136 process: * (SEMVER-MINOR) add process.getBuiltinModule(id) (Joyee Cheung) #52762 src: * (SEMVER-MINOR) traverse parent folders while running `--run` (Yagiz Nizipli) #53154 src,permission: * (SEMVER-MINOR) --allow-wasi & prevent WASI exec (Rafael Gonzaga) #53124 test_runner: * (SEMVER-MINOR) add snapshot testing (Colin Ihrig) #53169 * (SEMVER-MINOR) add context.fullName (Colin Ihrig) #53169 * (SEMVER-MINOR) support module mocking (Colin Ihrig) #52848 PR-URL: #53379
undici's MessageEvent is better tested and has a complete WebIDL implementation for validation. Not only this, but it's also used in Node's current WebSocket implementation. There are a large number of webidl-related issues in the current MessageEvent, such as not implementing `MessageEvent.prototype.initMessageEvent`, not validating arguments passed to its constructor (nodejs#51771), not validating the values passed to the constructor (such as not validating that `ports` is a sequence, not converting origin to a USVString, etc.), and other issues. fixup PR-URL: nodejs#52370 Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com>
Notable changes: buffer: * (SEMVER-MINOR) add .bytes() method to Blob (Matthew Aitken) nodejs#53221 cli: * (SEMVER-MINOR) add `NODE_RUN_PACKAGE_JSON_PATH` env (Yagiz Nizipli) nodejs#53058 * (SEMVER-MINOR) add `NODE_RUN_SCRIPT_NAME` env to `node --run` (Yagiz Nizipli) nodejs#53032 doc: * (SEMVER-MINOR) add context.assert docs (Colin Ihrig) nodejs#53169 * (SEMVER-MINOR) improve explanation about built-in modules (Joyee Cheung) nodejs#52762 * add StefanStojanovic to collaborators (StefanStojanovic) nodejs#53118 * add Marco Ippolito to TSC (Rafael Gonzaga) nodejs#53008 fs: * mark recursive cp methods as stable (Théo LUDWIG) nodejs#53127 lib: * (SEMVER-MINOR) add EventSource Client (Aras Abbasi) nodejs#51575 * (SEMVER-MINOR) replace MessageEvent with undici's (Matthew Aitken) nodejs#52370 module: * (SEMVER-MINOR) print amount of load time of a cjs module (Vinicius Lourenço) nodejs#52213 net: * (SEMVER-MINOR) add new net.server.listen tracing channel (Paolo Insogna) nodejs#53136 process: * (SEMVER-MINOR) add process.getBuiltinModule(id) (Joyee Cheung) nodejs#52762 src: * (SEMVER-MINOR) traverse parent folders while running `--run` (Yagiz Nizipli) nodejs#53154 src,permission: * (SEMVER-MINOR) --allow-wasi & prevent WASI exec (Rafael Gonzaga) nodejs#53124 test_runner: * (SEMVER-MINOR) add snapshot testing (Colin Ihrig) nodejs#53169 * (SEMVER-MINOR) add context.fullName (Colin Ihrig) nodejs#53169 * (SEMVER-MINOR) support module mocking (Colin Ihrig) nodejs#52848 PR-URL: nodejs#53379
undici's MessageEvent is better tested and has a complete WebIDL implementation for validation. Not only this, but it's also used in Node's current WebSocket implementation. There are a large number of webidl-related issues in the current MessageEvent, such as not implementing `MessageEvent.prototype.initMessageEvent`, not validating arguments passed to its constructor (nodejs#51771), not validating the values passed to the constructor (such as not validating that `ports` is a sequence, not converting origin to a USVString, etc.), and other issues. fixup PR-URL: nodejs#52370 Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com>
Notable changes: buffer: * (SEMVER-MINOR) add .bytes() method to Blob (Matthew Aitken) nodejs#53221 cli: * (SEMVER-MINOR) add `NODE_RUN_PACKAGE_JSON_PATH` env (Yagiz Nizipli) nodejs#53058 * (SEMVER-MINOR) add `NODE_RUN_SCRIPT_NAME` env to `node --run` (Yagiz Nizipli) nodejs#53032 doc: * (SEMVER-MINOR) add context.assert docs (Colin Ihrig) nodejs#53169 * (SEMVER-MINOR) improve explanation about built-in modules (Joyee Cheung) nodejs#52762 * add StefanStojanovic to collaborators (StefanStojanovic) nodejs#53118 * add Marco Ippolito to TSC (Rafael Gonzaga) nodejs#53008 fs: * mark recursive cp methods as stable (Théo LUDWIG) nodejs#53127 lib: * (SEMVER-MINOR) add EventSource Client (Aras Abbasi) nodejs#51575 * (SEMVER-MINOR) replace MessageEvent with undici's (Matthew Aitken) nodejs#52370 module: * (SEMVER-MINOR) print amount of load time of a cjs module (Vinicius Lourenço) nodejs#52213 net: * (SEMVER-MINOR) add new net.server.listen tracing channel (Paolo Insogna) nodejs#53136 process: * (SEMVER-MINOR) add process.getBuiltinModule(id) (Joyee Cheung) nodejs#52762 src: * (SEMVER-MINOR) traverse parent folders while running `--run` (Yagiz Nizipli) nodejs#53154 src,permission: * (SEMVER-MINOR) --allow-wasi & prevent WASI exec (Rafael Gonzaga) nodejs#53124 test_runner: * (SEMVER-MINOR) add snapshot testing (Colin Ihrig) nodejs#53169 * (SEMVER-MINOR) add context.fullName (Colin Ihrig) nodejs#53169 * (SEMVER-MINOR) support module mocking (Colin Ihrig) nodejs#52848 PR-URL: nodejs#53379
undici's MessageEvent is better tested and has a complete WebIDL
implementation for validation. Not only this, but it's also used in
Node's current WebSocket implementation. There are a large number of
webidl-related issues in the current MessageEvent, such as not
implementing
MessageEvent.prototype.initMessageEvent
, not validatingarguments passed to its constructor
(#51771), not validating the values
passed to the constructor (such as not validating that
ports
is asequence, not converting origin to a USVString, etc.), and other issues.