src: add encodeInto to TextEncoder #28862
Conversation
|
Is this a dupe of #28860 ? |
9059358 to
c4c5c56
Compare
|
@addaleax thanks for your help. It is my great pleasure to meet such kind reviewer. I learned a lot from you. I am enjoying the whole process. |
c4c5c56 to
a40ee62
Compare
a40ee62 to
d0e64e8
Compare
d0e64e8 to
e9809b0
Compare
There was a problem hiding this comment.
@addaleax @joyeecheung I am sure this commit only use string once. Please review, thank u. This indeed is more efficient but less readable. My way is a little ugly, I think there may be a more elegant way to implement encodeInto.
2285635 to
34cc8b5
Compare
|
@addaleax @joyeecheung I implemented it with only one copy. Is it ok? I am looking forward your reply, thank you. |
There was a problem hiding this comment.
This looks better in terms of performance, thank you :)
There was a problem hiding this comment.
Just some doc typos and consistency nits.
|
Ping @AtticusYang – are you still working on this? :) |
Add function encodeInto to TextEncoder, fix bug MessageChannel is not defined in encodeInto.any.js. Fixes: nodejs#28851 Refs: nodejs#26904
34cc8b5 to
91d61fb
Compare
I follow you and @jasnell @vsemozhetbyt tips and fix it, i pull a new request. |
|
@addaleax , thanks for you help, I don't find an elegant and effective way to solve the question, so i want to close the pull request.
|
|
@AtticusYang Do you mind if I open a new PR based on this one? This seemed quite close to being ready, tbh. |
@addaleax Not at all. Go ahead. |
Add function encodeInto to TextEncoder, and add MessageChannel to the encodeInto.any.js test. Fixes: nodejs#28851 Fixes: nodejs#26904 Refs: nodejs#28862 Co-authored-by: AtticusYang <yyongtai@163.com>
Add function encodeInto to TextEncoder, and add MessageChannel to the encodeInto.any.js test. Fixes: #28851 Fixes: #26904 Refs: #28862 Co-authored-by: AtticusYang <yyongtai@163.com> PR-URL: #29524 Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Add function encodeInto to TextEncoder, and add MessageChannel to the encodeInto.any.js test. Fixes: #28851 Fixes: #26904 Refs: #28862 Co-authored-by: AtticusYang <yyongtai@163.com> PR-URL: #29524 Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Add function encodeInto to TextEncoder, and add MessageChannel to the encodeInto.any.js test. Fixes: #28851 Fixes: #26904 Refs: #28862 Co-authored-by: AtticusYang <yyongtai@163.com> PR-URL: #29524 Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Add function encodeInto to TextEncoder, fix bug MessageChannel is not defined
in encodeInto.any.js.
Fixes: #28851
Refs: #26904
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes