Skip to content
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

Add support for Browser SDK #75

Merged
merged 1 commit into from
Apr 7, 2021
Merged

Conversation

surma
Copy link
Collaborator

@surma surma commented Apr 2, 2021

This adds an AMD build of the transform and an example show-casing how to use the transform with the Browser SDK.

It’s not clean by any means, but it’s a deeper issue with how the Browser SDK currently handles dynamic requires (as in: it expects them to be synchronously loaded). To work around this requirement, I’m using a Proxy to shim require() to give the transform some special handling. See the example HTML file for details (cc @dcodeIO for visibility and/or ideas).

@torch2424 This does work, so that’s pretty cool :D But you said you had a user who relies on the browser SDK. I’d be curious if this is acceptable for them.

@surma surma requested a review from torch2424 April 2, 2021 17:01
Copy link
Owner

@torch2424 torch2424 left a comment

Choose a reason for hiding this comment

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

Yooo! Thank you so much for taking the time and making a browser sdk example! 😄

I just have a single question / comment, the rest of the comments are just cool things I noticed :)

I'll also reach out to the as-bind user that I know of to take a look at this as well! 😄

// as is expected by most transform modules.
define("assemblyscript", assemblyscript);
// Load our ASBind transform...
require(["/dist/transform.amd.js"], transform => {
Copy link
Owner

Choose a reason for hiding this comment

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

Woooaaahhh that's interesting how you load a transform in the browser SDK 😯

@torch2424
Copy link
Owner

And I got a little antsy, so I tried it out, and it works! That's so rad 😄 🎉

Screenshot from 2021-04-02 18-03-43

@surma
Copy link
Collaborator Author

surma commented Apr 3, 2021

Alright, if you are conceptually on-board with this, then I’m happy! I’ll wait to hear from you about said as-bind user. If they are cool, I’ll merge this and make sure to cover this in the README update that’s next :D

@dcodeIO
Copy link

dcodeIO commented Apr 3, 2021

What if we'd add a transforms property to the options passed to asc.main, taking pre-required so to say transforms? Would that work, or does this depend on the --transform command line argument somehow?

@surma
Copy link
Collaborator Author

surma commented Apr 3, 2021

@dcodeIO yeah something like that would work well for my use-cases with transforms. Even just being allowed to push onto the transforms array.

Maybe @willemneal has thoughts on this, too, as he has spent a lot more time with transforms than I have.

@surma
Copy link
Collaborator Author

surma commented Apr 5, 2021

@torch2424 wdyt? Should we move forward with this?

@torch2424
Copy link
Owner

@surma My bad for the late reply! I've been moving work computers, so I was re-getting access to everything on my new machine 🙃

Alright, if you are conceptually on-board with this, then I’m happy! I’ll wait to hear from you about said as-bind user. If they are cool, I’ll merge this and make sure to cover this in the README update that’s next :D

So I did reach out to them, but they didn't get back to me. Though, since @dcodeIO made it really easy to include transforms, I think we could totally move forward with this! 😄

@torch2424
Copy link
Owner

@surma Let me know when this is good to merge. I know the CI is failing, but that's as expected right? 😄

@surma
Copy link
Collaborator Author

surma commented Apr 7, 2021

Yeah, I need to take a closer look at CI! Let’s merge this and I’ll whip up a new README 🎉

@torch2424 torch2424 merged commit d50a8d0 into type-embed Apr 7, 2021
@torch2424 torch2424 deleted the type-embed-browser-sdk branch April 7, 2021 23:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants