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

Jump to bottom

syscall/js: add Scope function #56084

Open
romaindoumenc opened this issue Oct 6, 2022 · 11 comments · May be fixed by #56867
Open

syscall/js: add Scope function #56084

romaindoumenc opened this issue Oct 6, 2022 · 11 comments · May be fixed by #56867
Labels
OS-JS Proposal Proposal-Accepted
Milestone
Backlog

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
@romaindoumenc romaindoumenc mentioned this issue Oct 6, 2022
Closed
@ianlancetaylor ianlancetaylor changed the title proposal: affected/package: syscall/js proposal: syscall/js: add Scope function Oct 6, 2022
@ianlancetaylor
Copy link
Contributor

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

@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

go/misc/wasm/wasm_exec_node.js

Lines 33 to 36 in 5b606a9

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 •
edited

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
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 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
@romaindoumenc romaindoumenc linked a pull request Nov 21, 2022 that will close this issue
Open
@gopherbot
Copy link

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
OS-JS Proposal Proposal-Accepted
Projects
Proposals
Status: Accepted
Milestone
Backlog
Development

Successfully merging a pull request may close this issue.

syscall/js: add Scope function TroutSoftware/go
5 participants
@neelance @rsc @ianlancetaylor @romaindoumenc @gopherbot