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

syscall/js: add Scope function #56084

Open
romaindoumenc opened this issue Oct 6, 2022 · 11 comments
Open

syscall/js: add Scope function #56084

romaindoumenc opened this issue Oct 6, 2022 · 11 comments

Comments

@romaindoumenc
Copy link

The current syscall/js package allow to interact with the global object (globalThis) only.

This has been reported as a problem in multiple instances, e.g. - #25612. Solutions to the problem have been so far in the “big stones” range (e.g. adopting ABI, …).

This proposal, instead, is squarely in the “small stone” category, but has worked well for us (using Go + WASM as a Web UI component), making sure that we are able to export functions in a limited scope only.

The proposal is two-and-an-optional fold:

  1. Passing an optional scope argument to the Go constructor, which can be an arbitrary object. This object is then exposed to the js / Go interface.
  2. This scope object is made available as a Scope() top-level function in the syscall/js package
  3. Since we are opening up that hatch, we also allow an optional array to contain environment variables, that allow caller to explicitly pass some extra arguments to the WASM code.

PR attach demonstrate the solution, that (seems to be) fully functional.

I’d like to the gauge the maintainers interest for a solution like this?

Many thank.

@gopherbot gopherbot added this to the Proposal milestone Oct 6, 2022
@ianlancetaylor ianlancetaylor changed the title proposal: affected/package: syscall/js proposal: syscall/js: add Scope function Oct 6, 2022
@ianlancetaylor ianlancetaylor moved this to Incoming in Proposals Oct 6, 2022
@ianlancetaylor
Copy link
Member

CC @neelance

@rsc
Copy link
Contributor

rsc commented Oct 12, 2022

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc rsc moved this from Incoming to Active in Proposals Oct 12, 2022
@neelance
Copy link
Member

In general, adding js.Scope() makes sense.

I would prefer to not add arguments to the constructor, but set it as a field before calling go.run:

go.scope = ...;

similar to

const go = new Go();
go.argv = process.argv.slice(2);
go.env = Object.assign({ TMPDIR: require("os").tmpdir() }, process.env);
go.exit = process.exit;

@romaindoumenc
Copy link
Author

Sure, either interface is fine (and, the JS side is not the most complicated to change anyway).

Based on the expressed interest, I’m happy to try to move this proposal forward, and commit it upstream.
Test-wise, would you prefer something in the existing js_test.go, or a browser-based version (e.g. using chromedriver ) ?

@rsc
Copy link
Contributor

rsc commented Oct 26, 2022

It sounds like the proposal is that on the JS side the go object gets a new field 'scope' that is checked, and on the Go side, in package syscall/js, func Scope() Value returns go.scope when it has been provided, or else window/global.

Are there any changes to what js.Global returns? The attached PR changes the comments but does not seem to update the behavior to match.

@romaindoumenc
Copy link
Author

romaindoumenc commented Oct 26, 2022

My bad, this is a leftover from a first stab at it (where we tried re-using the global object and overriding it with a scope).

Turns out, even with a scope, you still need the global object: e.g. in the browser, some events are only relevant on the globalThis object. You want to keep both.
Plus this means we have a backward-compatible API (I know syscall/js is not covered by the Go 1 compatibility guaranties, but there is little value here in breaking it anyway).

Which is a long-winded answer to say: sorry, ignore the comment, it went in by mistake. Scope is the only object to be added.

@rsc
Copy link
Contributor

rsc commented Nov 2, 2022

OK, so the proposal is:

  • on the JS side the go object gets a new field 'scope' that is checked
  • on the Go side, in package syscall/js, func Scope() Value returns go.scope when it has been provided, or else window/global.

Does anyone object to this proposal?

@rsc
Copy link
Contributor

rsc commented Nov 9, 2022

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

@rsc rsc moved this from Active to Likely Accept in Proposals Nov 9, 2022
@rsc
Copy link
Contributor

rsc commented Nov 16, 2022

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

@rsc rsc moved this from Likely Accept to Accepted in Proposals Nov 16, 2022
@rsc rsc changed the title proposal: syscall/js: add Scope function syscall/js: add Scope function Nov 16, 2022
@rsc rsc modified the milestones: Proposal, Backlog Nov 16, 2022
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/452356 mentions this issue: syscall/js: add Scope function

@romaindoumenc
Copy link
Author

@neelance : do you think we could get the review unlock for the 1.21 window? Thanks!

romaindoumenc added a commit to TroutSoftware/go that referenced this issue Jun 10, 2024
Fixes golang#56084

This changes allow to pass an arbitrary object from the JS world to the WASM program.

 - if no object is provided, globalThis is returned instead.
 - the scope object is derive from internal jsGo object, avoiding a possible ID clash when the scope is unset (so left as globalThis)
 - test uses a dummy object to perform a simple get/set action.

Change-Id: I71afd43f6b32d04b76c3760ac5b2cb6c0c9dbd73
GitHub-Last-Rev: 44a0f15
GitHub-Pull-Request: golang#56867
romaindoumenc pushed a commit to TroutSoftware/go that referenced this issue Aug 19, 2024
romaindoumenc pushed a commit to TroutSoftware/go that referenced this issue Aug 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Accepted
Development

Successfully merging a pull request may close this issue.

5 participants