Skip to content
This repository has been archived by the owner on Jun 6, 2024. It is now read-only.

Expose TextEncoder for use in workers #31

Merged
merged 8 commits into from
Jan 15, 2019

Conversation

tootedom
Copy link
Contributor

I noticed that when trying to use new TextEncoder() inside a worker, that it would resulting an a not defined error.

I've attempted to expose the text-encoding polyfil, in this pull request, which allows the use of TextEncoder within a worker.

@hankjacobs
Copy link
Contributor

Thanks for this! With the holidays, things are a little more hectic than usual. I’ll try to review when I have a moment but it may take a little longer than normal.

@tootedom
Copy link
Contributor Author

Hola,

No problem at all, totally understand about the holidays. It's the busy season.

cheers
/dom

@tootedom
Copy link
Contributor Author

I've updated the pull, as a noticed TextDecoder.decode() from the text-encoding npm module wasn't working (not sure why).

As a result I took the polyfil for the decode from golang/go#27295 (it only supports utf-8, maybe it's worth removing text-encoding module and also using the TextEncoder from the same go lang polyfil? - one less external dependency)

@hankjacobs hankjacobs closed this Jan 2, 2019
@hankjacobs hankjacobs reopened this Jan 2, 2019
@hankjacobs
Copy link
Contributor

Thanks for the contribution! I'm not opposed to using Go's implementation for both TextEncoder and TextDecoder even if it only supports utf-8 (better than no support IMO). That being said, I do have some licensing concerns. I'll look into it and get back to you.

@tootedom
Copy link
Contributor Author

tootedom commented Jan 3, 2019

Hi there,

I can now see why the TextDecoder from text-encoding isn't working (after some hacking locally) I'll see if I can work out a work around; as I see what you mean about the licensing from the go issue.

If my lack of is knowledge fails to get me a work around in the next couple of days I'll post here what I'm seeing.

Cheers
/dom

The result being the decode method ended up in creating an empty bytes view : https://github.com/inexorabletash/text-encoding/blob/master/lib/encoding.js#L1086.  The CustomTextDecoder here ensures the Uint8Array's buffer is an ArrayBuffer, resulting in TextDecoders method setting up bytes view correctly
@tootedom
Copy link
Contributor Author

tootedom commented Jan 3, 2019

Hola,

I've updated to monkey patch around the TextDecoder's decode method. The type checking at the beginning of the decode method was not recognising the internal buffer of the Uint8Array as an ArrayBuffer. The result being we ended up with bytes being a 0 array, and so TextDecoder.decode returned nothing.

The monkey patch, creates a new Uint8Array, with ArrayBuffer, setting contents from the original buffer. This results in TextDecoder's type checking working.

I'm not sure what the Root Cause of the missing type info is, maybe the vm runtime stuff. However, this does address it, and remove the need of that golang implementation.

Let me know your thoughts, cheers
/dom

lib/runtime.js Outdated Show resolved Hide resolved
lib/runtime/text-encoder.js Outdated Show resolved Hide resolved
lib/runtime/text-encoder.js Outdated Show resolved Hide resolved
lib/runtime/text-encoder.js Outdated Show resolved Hide resolved
@tootedom
Copy link
Contributor Author

tootedom commented Jan 9, 2019

Hola,

I've pushed the changes mentioned, and merged in current master. Let me know your thoughts.

Cheers
/Dom

@hankjacobs
Copy link
Contributor

Also, please resolve conflicts

@tootedom
Copy link
Contributor Author

Let me know if that latest change matches what we were talking about.
cheers
/dom

Copy link
Contributor

@hankjacobs hankjacobs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking great thus far. Some minor changes but we're in the home stretch. Thanks for all the hard work!

lib/runtime/__tests__/textencoder.test.js Outdated Show resolved Hide resolved
lib/runtime/__tests__/textencoder.test.js Outdated Show resolved Hide resolved
lib/runtime/__tests__/textencoder.test.js Outdated Show resolved Hide resolved
lib/runtime/__tests__/textencoder.test.js Outdated Show resolved Hide resolved
lib/runtime/text-encoder.js Outdated Show resolved Hide resolved
lib/runtime/text-encoder.js Outdated Show resolved Hide resolved
@tootedom
Copy link
Contributor Author

updated.
cheers
/dom

@hankjacobs hankjacobs merged commit 17a24b1 into dollarshaveclub:master Jan 15, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants