decouple vs/base/node/encoding.ts from node streams for #79275#99413
decouple vs/base/node/encoding.ts from node streams for #79275#99413bpasero merged 1 commit intomicrosoft:masterfrom gyzerok:docuple-encoding-from-node-streams
Conversation
package.json
Outdated
There was a problem hiding this comment.
I had to do it temporarily, so the types of iconv.getDecoder/iconv.getEncoder are there. However I believe we will change it before merging.
There are couple of routes we can take here:
- Use
iconv-lite-umdmodule, which will have correct type definitions. - Since the APIs are there and only types are not, we can temporarily use
as ...with our own type definitions.
Personally I prefer option 1.
Update
Since iconv-lite 0.6.0 got release, I've updated here to use it.
There was a problem hiding this comment.
A bit offtopic:
I was wondering why do you have to pass reducer to consumeStream? It is already being passed on stream creation. Why not reuse it?
There was a problem hiding this comment.
@gyzerok we still need the reducer because consumeStream is generic, right? Or would you suggest to have the reducer be accessible from the stream to reuse it?
There was a problem hiding this comment.
Yeah, I was thinking more conceptually, not from the types perspective. At least from my point of view it makes sense to change it so, that the reducer passed on the creation is reused. I assume it can either be done by either making consumeStream a method of ReadableStream or by making reducer accessible.
Anyway, consider it more as a suggestion from the "new to the project" person perspective :)
There was a problem hiding this comment.
I am trying very hard to have as little methods on the stream as possible to allow for easy inter-op with e.g. node.js or any other stream interface. So I do not want to push too many things onto it.
There was a problem hiding this comment.
The ITextSnapshot interface here does match Readable (but does not implement it). Is it accidental or intentional?
There was a problem hiding this comment.
@gyzerok historically the ITextSnapshot interface was first and then I added Readable, I think it is fine to leave it since TS takes care of understanding the interface is implemented by structural type-check.
There was a problem hiding this comment.
I am wondering if it makes sense to separately change the code so that ITextSnapshot implements Readable. Not as part of this change, but in general. Otherwise thanks for the explanation!
There was a problem hiding this comment.
I think it is fine like this given TS takes care, but thanks for noting.
|
I merged the wrong PR......... |
|
@gyzerok sorry for that, I am not entirely sure how to restore this PR, can you maybe do a new one from your branch? I will meanwhile review your changes and then can give feedback |
|
@gyzerok nevermind, this PR is incredibly well done given the complexity of the matter. I will push your changes to master and then push a change on top with some changes that I think are still needed and ping you on those changes and explain why I did them. The next step I guess is to flip to |
|
@bpasero thanks for the kind words! I think I can go with migrating to |
|
@gyzerok yeah I think you outlined it well, to summarise with my own words: all of the encoding related code within native text file service should move up to the We can also think about doing the encoding guessing as an independent step and leaving it as something the native service could pass over. Up to you. As I mentioned in #79275 (comment), I still think the dependency to |
2 participants
This PR introduces the first piece of work which is needed to get #79275 done. It was agreed in advance that Benjamin Pasero will take care of the review here.
Not sure yet if we are going to keep all the future changes in this PR and merge them all at once or merge this PR and open more for next steps. It all depends on Benjamin to decide.