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

Emit ES2020 builds for newer browsers, and emit ES5 for older browsers. #89

Merged
merged 3 commits into from
Nov 30, 2021

Conversation

12wrigja
Copy link
Contributor

Fixes #87.

@12wrigja 12wrigja marked this pull request as ready for review November 15, 2021 18:23
@12wrigja 12wrigja requested a review from justingrant November 15, 2021 18:23
@12wrigja
Copy link
Contributor Author

@justingrant I know we've had a bunch of problems around build artifact output, and while I've tried this out myself against serverless and a minimal React build I'd love to know if this change causes you any issues with your projects.

My understanding is that while it was common to ship ES5 in all the package bundles, there's no way to "un-transpile" if your target environment can use newer language versions, so there's a shift to ship newer code in packages? Does that match your understanding?

@justingrant
Copy link
Contributor

justingrant commented Nov 16, 2021

@justingrant I know we've had a bunch of problems around build artifact output, and while I've tried this out myself against serverless and a minimal React build I'd love to know if this change causes you any issues with your projects.

I'll try this out tomorrow.

My understanding is that while it was common to ship ES5 in all the package bundles, there's no way to "un-transpile" if your target environment can use newer language versions, so there's a shift to ship newer code in packages? Does that match your understanding?

Hmm, I'm not sure what the circa-2021 best practice is. Any idea how we'd find out?

Also, there's some Test262 failures. Expected?

@12wrigja
Copy link
Contributor Author

Also, there's some Test262 failures. Expected?

I'll look at these tomorrow - I was half expecting something to break but didn't feel like waiting around for the results... :)

R.e. best practices - I vaguely recall being sent a webpage a while back detailing various good OSS build setups, but I've mis-placed it. I'll see if I can find it. It was created by Chrome DevRel with the collaboration of the various packaging tool maintainers, so I'd hope it's reasonably up to date.

@12wrigja
Copy link
Contributor Author

Fixed the 262 tests - turns out they fail if we pass it transpiled ES5 output or optimized output. Different set of failures for both cases.

Do we care about these forms of failure? Some of the failures I think are expected:

  • the ES5 build fails a lot of the not-a-constructor tests simply because the transpiled form of various things is a function that can be constructed using Reflect.construct.
  • an optimized (but not transpiled) build seems to fail various strict mode checks (maybe this is fixable by adjusting Terser settings?)

@justingrant
Copy link
Contributor

Fixed the 262 tests - turns out they fail if we pass it transpiled ES5 output or optimized output. Different set of failures for both cases.

Do we care about these forms of failure? Some of the failures I think are expected:

  • the ES5 build fails a lot of the not-a-constructor tests simply because the transpiled form of various things is a function that can be constructed using Reflect.construct.

We discussed in today's champions' meeting. Consensus:

  1. Our npm release should be minified and transpiled, because polyfills can be dynamically included based on the user agent (not bundled) so keeping them small and broadly-usable is helpful.
  2. Polyfill code should try to pass Test262 in all build configurations, except tests that simply can't pass when the polyfill is transpiled too far back. For example, if new.target is unavailable, then we didn't think there's any way to get those not-a-constructor tests to pass.
  3. Therefore, the advice was to do have the prod polyfill run Test262 in two separate build configurations, both of which should pass on every PR:
    a. One with target of ES2020 where all Test262 tests should pass.
    b. One with target of (whatever our target for npm, e.g. ES5?) where some Test262 tests will fail (like those not-a-constructor tests) but we exclude them using expected-failures.txt.

One complexity with (3b) is that we will have expected-failures.txt sometimes anyways, so we probably need to dynamically assemble an expected-failures.txt file that combines the "expected failures due to transpilation" and "other expected failures unrelated to transpilation" where (3b) gets the union of those two files while (3a) only gets the latter file.

  • an optimized (but not transpiled) build seems to fail various strict mode checks (maybe this is fixable by adjusting Terser settings?)

We thought it would be good to understand these failures more and to see if we could adjust terser to prevent these failures without a large bad impact on bundle size. If that's not possible, then we could use the same expected-failures.txt solution above for this case too. Although consensus was that the minifier should be spec-compliant and we may need to file bugs against terser depending on what we find.

@12wrigja, what do you think about the above?

The other question I guess is how far back we want to transpile. ES5? Later? I don't have a strong opinion about this, but I assume there are some Google standards for this?

@12wrigja
Copy link
Contributor Author

there are some Google standards for this

Currently, the default transpilation output is ES5 for most things at Google. Some teams emit code at newer language versions if the browsers they support can run it.

Agreed r.e. Terser + the other Test262 setup - I'll try and look into this all soon.

We discussed in today's champions' meeting.

I didn't realize that this would get discussed given I had filed it so close to the meeting 😅

npm release should be minified and transpiled

For the UMD bundle, this makes sense. For ESM and CJS builds I don't think that makes sense given that if they are being dynamically included they wouldn't function properly anyways (and if they did then doesn't that mean the browser supports a language version > ES5)?

@justingrant
Copy link
Contributor

npm release should be minified and transpiled

For the UMD bundle, this makes sense. For ESM and CJS builds I don't think that makes sense given that if they are being dynamically included they wouldn't function properly anyways (and if they did then doesn't that mean the browser supports a language version > ES5)?

@12wrigja, this makes sense, but I don't know enough about the topic to agree or disagree. Are there folks at Google you could ask about this?

@pipobscure this was your feedback from this morning, want to add anything?

@justingrant
Copy link
Contributor

there are some Google standards for this

Currently, the default transpilation output is ES5 for most things at Google. Some teams emit code at newer language versions if the browsers they support can run it.

ES5 sounds reasonable.

@12wrigja
Copy link
Contributor Author

I've filed #91 to report the current state of 262 tests on that output, and to track improving it.

Getting the test suite to run against the optimized output is pretty easy but fixing the optimization problems seems to be quite difficult, so my current plan is to:

  • in this PR, implement the second 262 workflow and mark all tests that currently fail as "expected" for that run.
  • In a follow-up PR (probably after asking the Terser maintainers for guidance), try and reduce the number of cases that fail down based on things that could reasonably be fixed. There's bound to be some checks that won't ever pass (e.g. the not-a-constructor tests which use Reflect.construct on transpiled class methods and fail because, once transpiled, they are constructable when they should not be).

I still think this PR (with the second 262 run included) should block cutting releases, but the latter improvements should not. Thoughts?

@12wrigja 12wrigja requested a review from ptomato November 19, 2021 21:20
@justingrant
Copy link
Contributor

I still think this PR (with the second 262 run included) should block cutting releases, but the latter improvements should not. Thoughts?

Agreed. Sounds good. Thanks for digging into this.

@12wrigja
Copy link
Contributor Author

Ok - I've setup two more test262 runs - one for an "optimized" build (representative of the ESM/CJS bundle being optimized by Terser only), and an "es5" build (representative of the UMD bundle). There are separate expected-failures lists for each bundle, with expected-failures.txt always being used (e.g. to house tests that will fail for all test types e.g. due to bad mathematics, etc).

Copy link
Contributor

@ptomato ptomato left a comment

Choose a reason for hiding this comment

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

LGTM!

Another option that I didn't think of the other day, could be to not have expected-failures-es5.txt, and just set the test262-es5 job to not be required to pass. But this seems just as fine to me, and in fact more granular.

@12wrigja
Copy link
Contributor Author

I think I'd prefer to know that at least all the mathematical logic / other functionality works, even if for ES5 we can't get certain tests to pass that some users are probably not interested in (e.g. length of functions, the descriptor's name, etc).

Interestingly, it seems the ES5 version is faster as well? 7m vs 9m for the "debug" build (no optimization at all, just transpilation from TS to ES2020 JS).

@12wrigja
Copy link
Contributor Author

@justingrant Do you think you'll have time to look at these build artifacts in your projects at some point soon?

@justingrant
Copy link
Contributor

I built an npm package (using npm pack) from this PR, installed it into my main project, and then deployed and ran it. Didn't run into any problems. LGTM!

@12wrigja
Copy link
Contributor Author

Fantastic! I'm going to merge this, then sync up and new changes from the proposal and then we can release?

@12wrigja 12wrigja merged commit 2331468 into js-temporal:main Nov 30, 2021
@12wrigja 12wrigja deleted the fix_rollup_config branch November 30, 2021 00:43
@justingrant
Copy link
Contributor

Fantastic! I'm going to merge this, then sync up and new changes from the proposal and then we can release?

Yep, sounds good. Don't forget to merge #93 before release too. Thx!

@12wrigja 12wrigja restored the fix_rollup_config branch March 4, 2022 04:18
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.

UMD bundle is not correctly transpiled to ES5.
3 participants