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

Remove type() method from Tag object #318

Merged
merged 1 commit into from
Jul 16, 2024
Merged

Remove type() method from Tag object #318

merged 1 commit into from
Jul 16, 2024

Conversation

dschuff
Copy link
Member

@dschuff dschuff commented Jun 27, 2024

This method mirrors the type methods on the other interfaces (Global, Memory, etc) as defined in the JS type reflection proposal (https://github.com/WebAssembly/js-types/blob/main/proposals/js-types/Overview.md) Since this proposal will be standardized first, JS types should be rebased on top of exception-handling, and this method should be moved there.

This method mirrors the type methods on the other interfaces (Global, Memory,
etc) as defined in the JS type reflection proposal
(https://github.com/WebAssembly/js-types/blob/main/proposals/js-types/Overview.md)
Since this proposal will be standardized first, JS types should be rebased on top
of exception-handling, and this method should be moved there.
@dschuff
Copy link
Member Author

dschuff commented Jun 28, 2024

There is one important potential complication here:
2 1 of the 3 browsers have already shipped this method as part of the legacy exception proposal.
(even though none have shipped js-types yet, Chrome and Safari implement the type method, but Firefox does not).

That leaves a few options:

  1. Remove tag.type from the spec, and unship it from the web. This obviously has the potential in theory to break users today, but will ensure that we can change the JS types proposal before we ship that, if necessary.
  2. Keep Tag.type() implemented, and either:
    1. Keep it in the spec and commit to not breaking it. This would potentially limit our ability to change the JS types proposal before it ships, should we want to do that.
    2. Remove it from the spec for now. This essentially defers the risk of breaking the web: If the JS types proposal doesn't change before it ships, then nothing can break and the API will be how we want it. (But if we do decide we want to change it, then we need to decide whether to change Tag.type to match, at a potentially higher risk of breakage than we have today).

Practically speaking, I think all of these risks are very low. I haven't seen any code or users that have used Tag.type(), nor heard anyone complain about anything working on e.g. ChromeSafari but not Firefox because of this, so it seems unlikely that removing Tag.type() would break anyone. I also think there's little chance that we'll want to change the JS types proposal in a way that's incompatible with this (so the risk of keeping it is also low).
Anyone have thoughts?

@dtig @eqrion @ddegazio @SPY @aheejin @rossberg @Ms2ger @ajklein

@Ms2ger
Copy link
Contributor

Ms2ger commented Jul 4, 2024

Ugh, that's disappointing. I think the important question is what implementations will do:

a. Firefox also starts shipping this, matching Chrome and Safari (somewhat weirdly inconsistent api)
b. Nothing changes (no interop until the rest of js-types ships)
c. Chrome and Safari unship temporarily (small but perhaps existent compat risk, feels like busywork)

Possibly the path of least resistance to interop is a., if Firefox is willing to do that. In that case this PR should only remove the advisement. (If we go for c., I'd suggest creating a companion PR to add the method to js-types before merging this one.)

@ajklein
Copy link

ajklein commented Jul 8, 2024

I believe only Safari has shipped the type method without a flag guard. This can be seen on wpt.fyi. It matches my reading of the V8 code in Chromium, which adds the method only when the entire JS Types proposal is enabled.

So from Chrome's perspective, removing the type method from the spec seems like the right thing to do.

@dschuff
Copy link
Member Author

dschuff commented Jul 8, 2024

Oops, looks like I forgot I had the "experimental WebAssembly" flag enabled when I tested chrome 😅. I updated the OP

Copy link
Member

@aheejin aheejin left a comment

Choose a reason for hiding this comment

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

It sounds it is fine to be removed then. Can you land it before the CG meeting?

@dschuff dschuff merged commit 24be425 into main Jul 16, 2024
7 checks passed
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.

4 participants