Skip to content

Conversation

@sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Dec 14, 2023

Having these minimums in place will allows us to remove support for
older browsers from the codebase over time. For example, as a followup
to this change we should be able to remove out polyfills for Promise and
for Object.assign.

See #20601

@sbc100 sbc100 requested review from RReverser and juj December 14, 2023 19:25
@sbc100 sbc100 force-pushed the min_browser_versions branch 2 times, most recently from 4e7b854 to 965664e Compare December 14, 2023 19:34
@sbc100 sbc100 requested a review from kripken December 14, 2023 19:34
@sbc100 sbc100 force-pushed the min_browser_versions branch from 965664e to 02a61e2 Compare December 14, 2023 19:52
@RReverser
Copy link
Collaborator

This seems like an answer to #20601, but it doesn't seem the discussion there came to any conclusion.

Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

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

Can we not pass "0" to Babel, which I would (perhaps overly optimistically) hope tells it to support as far back as it possibly can?

@sbc100
Copy link
Collaborator Author

sbc100 commented Dec 14, 2023

Can we not pass "0" to Babel, which I would (perhaps overly optimistically) hope tells it to support as far back as it possibly can?

I'm not totally sure if that works or not. I think maybe it does? But I still think this approach is better as it allows us to move forward to removing code that we know we don't need.

@sbc100
Copy link
Collaborator Author

sbc100 commented Dec 14, 2023

Can we not pass "0" to Babel, which I would (perhaps overly optimistically) hope tells it to support as far back as it possibly can?

I'm not totally sure if that works or not. I think maybe it does? But I still think this approach is better as it allows us to move forward to removing code that we know we don't need.

By experimentation I think you are correct.. Putting 0 in the babel config works.

@RReverser
Copy link
Collaborator

I believe you should just not pass target, then it defaults to transpiling everything to ES5.

@sbc100 sbc100 force-pushed the min_browser_versions branch from 02a61e2 to 05ba997 Compare December 14, 2023 21:20
@sbc100
Copy link
Collaborator Author

sbc100 commented Dec 14, 2023

I believe you should just not pass target, then it defaults to transpiling everything to ES5.

I think this PR does better than that though, since its precise.

@sbc100 sbc100 force-pushed the min_browser_versions branch from 05ba997 to 389014b Compare December 14, 2023 21:21
@kripken
Copy link
Member

kripken commented Dec 14, 2023

I don't see the benefit of the extra precision, myself. Just letting Babel do its best as far back as it can seems good enough? @sbc100 what do you see as the advantages of knowing the specific versions?

@sbc100
Copy link
Collaborator Author

sbc100 commented Dec 14, 2023

I don't see the benefit of the extra precision, myself. Just letting Babel do its best as far back as it can seems good enough? @sbc100 what do you see as the advantages of knowing the specific versions?

If you give babel the precise information it can just perform the need transformations for the specific browsers. Or are you talking specifically about the -sLEGACY_VM_SUPPORT path? I'd be OK with leaving those as zero if you think that is cleaner?

@kripken
Copy link
Member

kripken commented Dec 15, 2023

If you give babel the precise information it can just perform the need transformations for the specific browsers. Or are you talking specifically about the -sLEGACY_VM_SUPPORT path? I'd be OK with leaving those as zero if you think that is cleaner?

Maybe I'm confused then. What is the difference? I mean that I only see these 3 situations, which we handle before this PR:

  • By default we have specific minimum browser versions, e.g. Chrome 85.
  • A user can adjust those settings, e.g. to Chrome 75, or to "0" to support as far back as possible.
  • A user can set LEGACY_VM_SUPPORT to declare maximum backwards compat in all browsers, which is the same as "0" for them all.

All those seem to be as precise as possible already, letting Babel do its thing. Which of them does this PR improve? Or is there a 4th situation I am missing?

@sbc100 sbc100 force-pushed the min_browser_versions branch from 389014b to c78485d Compare December 15, 2023 00:31
@sbc100
Copy link
Collaborator Author

sbc100 commented Dec 15, 2023

If you give babel the precise information it can just perform the need transformations for the specific browsers. Or are you talking specifically about the -sLEGACY_VM_SUPPORT path? I'd be OK with leaving those as zero if you think that is cleaner?

Maybe I'm confused then. What is the difference? I mean that I only see these 3 situations, which we handle before this PR:

  • By default we have specific minimum browser versions, e.g. Chrome 85.
  • A user can adjust those settings, e.g. to Chrome 75, or to "0" to support as far back as possible.
  • A user can set LEGACY_VM_SUPPORT to declare maximum backwards compat in all browsers, which is the same as "0" for them all.

All those seem to be as precise as possible already, letting Babel do its thing. Which of them does this PR improve? Or is there a 4th situation I am missing?

I was thinking that we could improve the 3rd case but it seems I was wrong, so I've reverted the 0 -> OLDEST_SUPPORTED_XX part if this change. Now LEGACY_VM_SUPPORT and MIN_XXX_VERION=0 maintina the zero value, including when passed to babel.

@sbc100 sbc100 force-pushed the min_browser_versions branch from c78485d to ac514ee Compare December 15, 2023 00:32
@kripken
Copy link
Member

kripken commented Dec 15, 2023

Are you saying you thought to help the 3rd case and reverted that - what benefit is meant to be left, then? I took another look at the code, and I still don't understand how it improves on the 3 cases, sorry - I feel like I'm missing something.

@kripken
Copy link
Member

kripken commented Dec 15, 2023

What I am still asking, basically is: why add these minimum browser versions, if they do not help the 3 cases I mentioned? Is the benefit elsewhere?

@sbc100
Copy link
Collaborator Author

sbc100 commented Dec 15, 2023

What I am still asking, basically is: why add these minimum browser versions, if they do not help the 3 cases I mentioned? Is the benefit elsewhere?

See #20601. Basically setting min versions allows us to, over time, remove workarounds and hacks that exist only to support certain older browser versions (e.g. the Promise polyfill) and do this in a principled way.

A good example, with changes like #20925 it would be more principled if we could say for sure that we catigorically don't support chrome versions older than 37.

@sbc100 sbc100 force-pushed the min_browser_versions branch from ac514ee to 3124b17 Compare December 15, 2023 01:29
@RReverser
Copy link
Collaborator

A good example, with changes like #20925 it would be more principled if we could say for sure that we catigorically don't support chrome versions older than 37.

Hah, ironically, #20925 and few more similar workarounds I saw in html5 and webgl libs was precisely the reason I started that discussion that branched off into #20601.

Would love to see some clear decision made there.

@sbc100
Copy link
Collaborator Author

sbc100 commented Dec 15, 2023

A good example, with changes like #20925 it would be more principled if we could say for sure that we catigorically don't support chrome versions older than 37.

Hah, ironically, #20925 and few more similar workarounds I saw in html5 and webgl libs was precisely the reason I started that discussion that branched off into #20601.

Would love to see some clear decision made there.

Do you object to the PR as a starting point though? It seems like we can bikeshed the precise policy and precise current limits later, right? This change is just designed as just the first step.

@RReverser
Copy link
Collaborator

Do you object to the PR as a starting point though?

Not to the versions, no - personally, the higher the easier - but I would start by documenting minimal versions we support in public docs, and then enforcing them in code, rather than other way around, to make sure there are no surprises.

It would also be good to get confirmation those minimal versions look good from eg @brionv and @juj too (and anyone else who works on projects large enough to potentially care about very old browsers).

Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

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

Rationale makes sense to me. lgtm % comments.

@sbc100 sbc100 force-pushed the min_browser_versions branch from 3124b17 to a8c7965 Compare December 18, 2023 23:40
Having these minimums in place will allows us to remove support for
older browsers from the codebase over time.  For example, as a followup
to this change we should be able to remove out polyfills for Promise and
for Object.assign.

Fixes: emscripten-core#20601
@sbc100 sbc100 force-pushed the min_browser_versions branch from a8c7965 to 02ce44d Compare December 18, 2023 23:43
@sbc100 sbc100 enabled auto-merge (squash) December 19, 2023 00:46
@sbc100 sbc100 disabled auto-merge December 19, 2023 00:58
@sbc100 sbc100 merged commit f3c8edc into emscripten-core:main Dec 19, 2023
@sbc100 sbc100 deleted the min_browser_versions branch December 19, 2023 00:58
sbc100 added a commit that referenced this pull request Dec 19, 2023
sbc100 added a commit to sbc100/emscripten that referenced this pull request Jan 3, 2025
These polyfills were only used for browsers that are no longer supported
by emscripten.  See emscripten-core#20924.
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