Amphtml: Make `esm` the default mode for AMP builds and tests

Created on 24 Mar 2020  路  8Comments  路  Source: ampproject/amphtml

There are several things we could potentially do for esm:

  • [ ] Enable esm unminified builds
  • [x] Enable esm minified builds
  • [x] Add esm support to gulp serve (#27340)
  • [x] Run integration tests against esm code in CI (#26556)
  • [ ] Run unit tests against esm code in CI (#26556)
  • [ ] Run e2e tests against esm compiled code in CI
  • [ ] Run visual diff tests against esm compiled code in CI
  • [ ] Run experiment A/B/C tests against esm compiled code in CI
  • [ ] Ensure code coverage still works for esm code
  • [x] Ensure error reporting still works for esm code (https://github.com/ampproject/amphtml/pull/27410)
  • [x] Ensure performance measurements still work for esm code (https://github.com/ampproject/amphtml/commit/b7c885318a8d609d8fd29d62d169db8ca0ee2eed#diff-cb8edaa6eb9ac9247ac010ab11cb1fa5ac23c9326f4e4f35fff3e62fbf87e192R104-R106)
  • [ ] Switch the default unminified mode to esm
  • [ ] Switch the default minified mode to esm

For more context around this effort, see https://github.com/ampproject/amphtml/issues/27386#issuecomment-604664640 and https://github.com/ampproject/amphtml/issues/27386#issuecomment-604674651.

Bug infra performance

Most helpful comment

Spoke to @erwinmombay today to get some more context. The high level goal is to increase confidence in the esm build as we start serving it to users. Once that happens, and once the number of users who get module (vs. no-module) due to their use of a modern browser increases, we should gradually do more of our default testing on esm builds.

I've pared down the list in the OP by removing a few items:

  • Generating .mjs and .js at one time (too inefficient)
  • Running esm tests on single_pass builds (performance WG has other plans)
  • Removing support for non-esm builds (we won't stop serving this, so we should continue testing)

A good end goal for this effort would be that tests are run by default on esm builds, and non-esm builds continue to get some testing.

I'll let @erwinmombay and @kristoferbaxter fill in more context and correct me if I'm wrong about anything.

All 8 comments

/cc @ampproject/wg-infra, @ampproject/wg-runtime, and @ampproject/wg-performance in case anyone has comments / corrections.

Last TODO item - switch default minified mode?

Other suggestions:

  • Run e2e, visual diff tests, single pass tests against esm compiled code in CI builds
  • Run unit tests against esm uncompiled code in CI builds
  • Run experiment tests against esm compiled code in CI master builds
  • Ensure code cov still works
  • Plan to phase out / delete non-esm mode?

@estherkim Added all your ideas to the original post. Thank you!

For those of us lacking context, do we have some sharable stated goals for this effort?

E.g. run tests against module build by default makes sense since lion's share of browsers will be running it, but will run all tests twice? Re: "esm unminified", do we keep using browserify for bundling?

Spoke to @erwinmombay today to get some more context. The high level goal is to increase confidence in the esm build as we start serving it to users. Once that happens, and once the number of users who get module (vs. no-module) due to their use of a modern browser increases, we should gradually do more of our default testing on esm builds.

I've pared down the list in the OP by removing a few items:

  • Generating .mjs and .js at one time (too inefficient)
  • Running esm tests on single_pass builds (performance WG has other plans)
  • Removing support for non-esm builds (we won't stop serving this, so we should continue testing)

A good end goal for this effort would be that tests are run by default on esm builds, and non-esm builds continue to get some testing.

I'll let @erwinmombay and @kristoferbaxter fill in more context and correct me if I'm wrong about anything.

@choumx had a good conversation with @rsimha today.

i'd like to say that we'd ideally run both module and nomodule build with the same test matrix, but that would be too inefficient as it stands with resources and capacity.

I'd like to prioritize a few things:

  • both module and nomodule builds running against AMP integration tests
  • unit tests switched over to module build (at some point)
  • visual diff tests switched over to module build
  • e2e tests switched over to module build

i think at some point we need to get rid of browserify + babel for the dev build and move over to rollup + babel as this gives us an avenue to use it (rollup) for the production pipeline, but that is i think a longer conversation.

Only running high-level tests for no-module sounds good. But I forget -- do our visual diff/e2e tests support running on IE? 馃槄

Might be a good idea to split coverage strategy into two:

  1. Make sure we don't break the no-module build itself (e.g. run visual diff tests on no-module in Chrome)
  2. Make sure we don't break AMP on no-module browsers (e.g. run very simple sanity tests on IE)

The biggest risk here I think is breaking UC browser which we officially support (unlike IE). @rsimha WDYT about a test coverage matrix one-pager for this?

do our visual diff/e2e tests support running on IE?

Visual diff needs Percy support for IE, and it's unlikely to ever happen. It might be possible to run E2E tests on IE with something like this, but we haven't tried it yet. (We added integration tests support for IE, but in two years, a grand total of one test has been enabled. So it's low priority for now.)

@danielrozenberg and @estherkim can keep me honest here.

WDYT about a test coverage matrix one-pager for this?

Will do. I'll mention these requests as well.

Was this page helpful?
0 / 5 - 0 ratings

Related issues

devanes picture devanes  路  3Comments

gmajoulet picture gmajoulet  路  3Comments

mrjoro picture mrjoro  路  3Comments

choumx picture choumx  路  3Comments

aghassemi picture aghassemi  路  3Comments