1/4 : Browser support - Githubissues

dbousque commented 5 years ago

Hi there !

This PR brings browser support. I am currently porting jaeger-node to the browser. As part of this effort, I'd like thriftrw to be browser compatible as well.

There aren't a lot of incompatibilities. In fact, the library code per se works well without any change. The only thing that is missing is file system access (see the added documentation for ways around that). Issues when bundling only arise with dependencies of thriftrw-node, which explains the few tricks used in browserify.js.

I'v tried to make as few changes as possible, and none were needed for the library, only the tests have been adapted a bit, so that they could run in a browser.

As far as the release model is concerned, I chose to keep both versions in the same package, leveraging the browser field in package.json, which allows bundle tools to use alternative package versions destined for browsers. If you prefer, we could have a separate package.

Let me know if you'd like more details or require some changes !

CLAassistant commented 5 years ago

All committers have signed the CLA.

dbousque commented 5 years ago

Thanks for the feedback and the bufrw release @kriskowal. I am not familiar with the thrift protocol. If I understand correctly, you'd like to give the ability to read sources asynchronously, given that users provide an asynchronous method for reading sources in the fs object (that they would implement, and so they could make HTTP requests for example). Is that correct ?

kriskowal commented 5 years ago

Thanks for the feedback and the bufrw release @kriskowal. I am not familiar with the thrift protocol. If I understand correctly, you'd like to give the ability to read sources asynchronously, given that users provide an asynchronous method for reading sources in the fs object (that they would implement, and so they could make HTTP requests for example). Is that correct ?

That is correct, though getting the API right may require experimentation. Ideally:

There is a common API regardless of whether it’s backed by Node.js or a web fetch.
All Thrift models share a common memo table and that table can be grown asynchronously and relinked incrementally.

Something that might work: perhaps constructing Thrift() with just an fs argument creates all of the shared memo tables, as it does today, but doesn’t require an entry point or express permission to use the filesystem. Passing the filesystem is an “object capability”. Permission to use it is implied by having been given a reference to it.

From there, perhaps we expose an asynchronous load function that accepts an entryPoint calls back with the corresponding Thrift model instance.

If this only needs to work in modern JavaScript, I would memoize load on a shared table of module paths to promises for the corresponding Thrift object that resolve when the module is loaded, its transitive dependencies have also loaded, and the entire working set has been linked. If it needs to work on Node.js 0.10 (unlikely), the same can be accomplished with the usual ceremony involving loaded and loading state maps and lists of subscribers to the loaded event.

I believe that the link operation is already idempotent so it can safely be applied to incrementally link new modules as they are found.

I hope this makes sense to you.

kriskowal commented 5 years ago

I should clarify, this all hinges on the fact that Thrift modules, just like JavaScript modules, can be loaded asynchronously but must be linked (executed) synchronously. You can read and compile a Thrift file just to read its include directives. They won’t be linkable until the module’s transitive dependencies have all loaded.

dbousque commented 5 years ago

Hi @kriskowal, thanks for the additional details. Hopefully you had a nice time away :) Sorry for the late work on this, I've been a bit busy lately.

I added the ability to load sources asynchronously. It works by adding an AST building step before compilation. It was way easier to do it this way instead of modifying the compilation step to be able to be asynchronous, as I wanted to maintain backwards compatibility, and adding promises without disturbing the guarantee to have a fully built model after the constructor is done running was a bit tricky. In fact, I had to do some (a few lines worth of code) code duplication for both the asynchronous and the synchronous case. If it's ok to do some breaking changes, we could have a cleaner internal implementation if we could let go of the guarantee to get a fully built model when the constructor is done running (in the synchronous case).

Some changes that were done :

Passing an entryPoint is no longer necessary, one can use the thrift.load() method to start the actual compilation.
Allow to pass an object with a load field as the fs option parameter to the main Thrift() constructor : if passed, the load function will be used to fetch sources and will be expected to return a promise.
Drop support for node 0.10 and start support from 0.12, because of the fact that native promises are missing. We can use a third-party promise library if necessary.
The tests were modified to run twice : once with synchronous source loading, once with asynchronous source loading.

I don't know if you know about it, but there is an option to ignore whitespace changes when reviewing on GitHub, it can be handy in this case as most changes are whitespace changes.

dbousque commented 5 years ago

Hi @kriskowal. I've brought back support for 0.10, removed the use of promises and used callbacks instead, and followed your naming recommendations. I've split the PR in 4, so as to make reviewing easier, here they are :

1. Browser support : https://github.com/thriftrw/thriftrw-node/pull/165 (it is the PR we are currently speaking on).
1. Support for asynchronous source loading : https://github.com/dbousque/thriftrw-node/pull/1
1. Tests for 2. : https://github.com/dbousque/thriftrw-node/pull/2
1. Licence bump : https://github.com/dbousque/thriftrw-node/pull/3

Have a nice day.

kriskowal commented 5 years ago

Thanks @dbousque. I’ve looped in engineers for review from the two areas where this code runs in production. I’ll try to find some time to spare to write a close review this week.

dbousque commented 5 years ago

Thanks a lot @kriskowal.

kriskowal commented 5 years ago

Approval contingent on test and coverage passing when the full stack is merged into this change.

dbousque commented 4 years ago

Hi @kriskowal, I don't know if you saw, I addressed the issues raised in https://github.com/dbousque/thriftrw-node/pull/1. I'd understand if it's a bit much to review. For my purposes, browser support without asynchronous source loading is enough. Maybe we can merge this PR at least?

kriskowal commented 4 years ago

I’ve landed this change, please go ahead and introduce the next change in your stack.

dbousque commented 4 years ago

Neat! I created the PR for the asynchronous source loading feature : https://github.com/thriftrw/thriftrw-node/pull/168. You can see old comments on this on the old PR : https://github.com/dbousque/thriftrw-node/pull/1. It's expected that the tests don't pass, as there is a separate tests PR : https://github.com/dbousque/thriftrw-node/pull/2. I can merge both into a single PR if you'd prefer.

dbousque commented 4 years ago

The PR has been adjusted since your latests comments. Let me know if you'd like the async source loading PR and the tests PR to be merged into a single one.

thriftrw / thriftrw-node

1/4 : Browser support #165