build-system: consolidate all build time constants #34327

samouri · 2021-05-11T20:14:33Z

summary
This PR consolidates all of our build-time constants into a single file and babel transform. My hope is that this PR makes it easier to debug what our build constants are set to in each of our modes.

Transforms removed:

is_fortesting-constant-transformer
is_minified-constant-transformer
transform-version-call
transform-internal-version

Notably this PR retains amp-mode-transform since it saves ~0.88kb from v0.mjs (as well as 0.1kb in most other binaries). What that transform does is replace any calls to getMode().minified, getMode().localDev and getMode().development to false in minified builds, often saving the need to import any code related to mode.

additional info

Originally I tried to move this work to the bundlers. It is more standard to have bundlers insert build-time constants than for babel to do it. This is partially due to their built-in features like DCE. Closure also emits uselessCode warnings that we must suppress when constant replacement is done by a pre-closure babel transform.

Unfortunately, closure doesn't support constant replacement in module mode (google/closure-compiler#1601), so I downscoped the idea to just consolidating our constants to a single location.

next steps

Once this lands, it should be pretty trivial to make improvements to the amp-mode-transformer to minify all calls to the build constants.
It would be nice to reuse options object passed throughout build-system instead of duplicating those calculations here.

amp-owners-bot · 2021-07-08T14:06:06Z

Hey @erwinmombay! These files were changed:

build-system/babel-plugins/babel-plugin-is_fortesting-constant-transformer/index.js
build-system/babel-plugins/babel-plugin-is_fortesting-constant-transformer/test/fixtures/transform-assertions/isdev-transform/input.js
build-system/babel-plugins/babel-plugin-is_fortesting-constant-transformer/test/fixtures/transform-assertions/isdev-transform/options.json
build-system/babel-plugins/babel-plugin-is_fortesting-constant-transformer/test/fixtures/transform-assertions/isdev-transform/output.js
build-system/babel-plugins/babel-plugin-is_fortesting-constant-transformer/test/fixtures/transform-assertions/no-transform/input.js
build-system/babel-plugins/babel-plugin-is_fortesting-constant-transformer/test/fixtures/transform-assertions/no-transform/options.json
build-system/babel-plugins/babel-plugin-is_fortesting-constant-transformer/test/fixtures/transform-assertions/no-transform/output.js
build-system/babel-plugins/babel-plugin-is_fortesting-constant-transformer/test/index.js
build-system/babel-plugins/babel-plugin-is_minified-constant-transformer/index.js
build-system/babel-plugins/babel-plugin-is_minified-constant-transformer/test/fixtures/transform-assertions/isminified-transform/input.js
build-system/babel-plugins/babel-plugin-is_minified-constant-transformer/test/fixtures/transform-assertions/isminified-transform/options.json
build-system/babel-plugins/babel-plugin-is_minified-constant-transformer/test/fixtures/transform-assertions/isminified-transform/output.js
+17 more

Hey @jridgewell! These files were changed:

build-system/babel-plugins/babel-plugin-is_fortesting-constant-transformer/index.js
build-system/babel-plugins/babel-plugin-is_fortesting-constant-transformer/test/fixtures/transform-assertions/isdev-transform/input.js
build-system/babel-plugins/babel-plugin-is_fortesting-constant-transformer/test/fixtures/transform-assertions/isdev-transform/options.json
build-system/babel-plugins/babel-plugin-is_fortesting-constant-transformer/test/fixtures/transform-assertions/isdev-transform/output.js
build-system/babel-plugins/babel-plugin-is_fortesting-constant-transformer/test/fixtures/transform-assertions/no-transform/input.js
build-system/babel-plugins/babel-plugin-is_fortesting-constant-transformer/test/fixtures/transform-assertions/no-transform/options.json
build-system/babel-plugins/babel-plugin-is_fortesting-constant-transformer/test/fixtures/transform-assertions/no-transform/output.js
build-system/babel-plugins/babel-plugin-is_fortesting-constant-transformer/test/index.js
build-system/babel-plugins/babel-plugin-is_minified-constant-transformer/index.js
build-system/babel-plugins/babel-plugin-is_minified-constant-transformer/test/fixtures/transform-assertions/isminified-transform/input.js
build-system/babel-plugins/babel-plugin-is_minified-constant-transformer/test/fixtures/transform-assertions/isminified-transform/options.json
build-system/babel-plugins/babel-plugin-is_minified-constant-transformer/test/fixtures/transform-assertions/isminified-transform/output.js
+19 more

samouri · 2021-07-08T14:11:14Z

src/internal-version.js

@@ -24,5 +24,5 @@
 * @return {string}
 */
 export function internalRuntimeVersion() {
-  return '$internalRuntimeVersion$';
+  return INTERNAL_RUNTIME_VERSION; // eslint-disable-line no-undef


note: I could also add this to the eslint config, but wasn't sure if that were preferable

I think the most unified solution, in keeping with the all-constants-in-one-file angle, would be to have a similar single forbidden-terms rule that uses one regex to test for /INTERNAL_RUNTIME_VERSION|IS_(FORTESTING|ESM|SXG|MINIFIED)/ and has a very short list of these files that use it (and maybe this particular file gets moved into #core/mode after this PR). That way if we ever need to adjust build constants we can easily find a list of files that can use them.

This reverts commit 853c8da.

build-system/compile/build-constants.js

build-system/test-configs/forbidden-terms.js

rcebulko · 2021-07-08T14:30:11Z

src/internal-version.js

@@ -24,5 +24,5 @@
 * @return {string}
 */
 export function internalRuntimeVersion() {
-  return '$internalRuntimeVersion$';
+  return INTERNAL_RUNTIME_VERSION; // eslint-disable-line no-undef


I think the most unified solution, in keeping with the all-constants-in-one-file angle, would be to have a similar single forbidden-terms rule that uses one regex to test for /INTERNAL_RUNTIME_VERSION|IS_(FORTESTING|ESM|SXG|MINIFIED)/ and has a very short list of these files that use it (and maybe this particular file gets moved into #core/mode after this PR). That way if we ever need to adjust build constants we can easily find a list of files that can use them.

rcebulko · 2021-07-08T14:34:01Z

build-system/babel-config/helpers.js

+    const replacement =
+      typeof value === 'boolean'
+        ? {type: 'booleanLiteral', value}
+        : {type: 'stringLiteral', value};


For your consideration; typechecking guarantees the possible results from typeof

Suggested change

const replacement =

typeof value === 'boolean'

? {type: 'booleanLiteral', value}

: {type: 'stringLiteral', value};

const replacement = {value, type: `${typeof value}Literal`};

Very clever! I don't know how I feel about this 🤔

Co-authored-by: Ryan Cebulko <ryan@cebulko.com>

samouri · 2021-07-08T14:46:30Z

I think the most unified solution, in keeping with the all-constants-in-one-file angle, would be to have a similar single forbidden-terms rule that uses one regex to test for /INTERNAL_RUNTIME_VERSION|IS_(FORTESTING|ESM|SXG|MINIFIED)/ and has a very short list of these files that use it (and maybe this particular file gets moved into #core/mode after this PR). That way if we ever need to adjust build constants we can easily find a list of files that can use them.

Good ideas all around. I've made the relevant changes to this PR.

build-system/test-configs/forbidden-terms.js

rsimha

Thanks for simplifying our code! LGTM with one non-blocking suggestion.

rsimha · 2021-07-09T17:17:58Z

src/internal-version.js

@@ -24,5 +24,5 @@
 * @return {string}
 */
 export function internalRuntimeVersion() {
-  return '$internalRuntimeVersion$';


For consistency, would it make sense to also remove the '$internalRuntimeVersion$' sentinel from all the call-sites in unit tests, etc. and replace it with INTERNAL_RUNTIME_VERSION?

https://github.com/ampproject/amphtml/search?q=%22%24internalRuntimeVersion%24%22

This might make it easier for a new engineer to search through the code while writing / debugging tests.

If you think it's worth it, doesn't have to be a part of this PR.

Are you proposing we also do the var replacement at test time? If so, I agree. Seems like '$internalRuntimeVersion$' is purely vestigial

Exactly what I was thinking. IIRC, the only reason test code uses '$internalRuntimeVersion$' is because src/internal-version.js was using it. The sentinel can be anything we want it to be.

Ok great. Will pursue this in a followup, since I can then do other related work in parallel

samouri mentioned this pull request May 11, 2021

draft: move all build constants for bundlers instead of babel #34277

Closed

samouri self-assigned this May 12, 2021

samouri force-pushed the build-consts2 branch from cb5296c to 47f9888 Compare July 6, 2021 01:03

samouri changed the title ~~babel constants: combine all the replacePlugins~~ babel constants: consolidate all build time constants Jul 7, 2021

samouri force-pushed the build-consts2 branch 2 times, most recently from 1923485 to bd1e3f9 Compare July 7, 2021 15:12

samouri changed the title ~~babel constants: consolidate all build time constants~~ build-system: consolidate all build time constants Jul 7, 2021

samouri force-pushed the build-consts2 branch 4 times, most recently from 8672968 to 853c8da Compare July 8, 2021 04:01

samouri marked this pull request as ready for review July 8, 2021 14:05

amp-owners-bot bot requested a review from estherkim July 8, 2021 14:06

samouri requested review from rsimha and jridgewell and removed request for estherkim July 8, 2021 14:08

samouri commented Jul 8, 2021

View reviewed changes

samouri and others added 10 commits July 8, 2021 10:32

Consolidate all babel replacements

73f0631

fix bs tsconfig

b0ee8cf

also remove internal-runtime-version

b305587

Fix constant selection, modify internal-version to use indent.

5a1f7c1

minify replaced in getMode....why

46a6347

Minify-replace does not work when the vars are declared.

037d6c4

remove amp-mode-transform

1ef5a02

Revert "remove amp-mode-transform"

bd763ac

This reverts commit 853c8da.

bring back the amp-mode minified transform

9c49155

revert seemingly unrelated change

79d99f6

rcebulko reviewed Jul 8, 2021

View reviewed changes

samouri force-pushed the build-consts2 branch from 9a4bb91 to 79d99f6 Compare July 8, 2021 14:40

Update build-system/compile/build-constants.js

e4fde0c

Co-authored-by: Ryan Cebulko <ryan@cebulko.com>

samouri added 2 commits July 8, 2021 10:52

address rcebulko feedback

205a558

forbidden terms message update

fb2b14e

rcebulko approved these changes Jul 8, 2021

View reviewed changes

build-system/test-configs/forbidden-terms.js Outdated Show resolved Hide resolved

further cleanup

0cf1a3f

samouri mentioned this pull request Jul 8, 2021

wg-performance Status Update 2021-07-07 ampproject/wg-performance#87

Closed

samouri enabled auto-merge (squash) July 8, 2021 15:59

samouri disabled auto-merge July 8, 2021 19:27

rsimha approved these changes Jul 9, 2021

View reviewed changes

jridgewell approved these changes Jul 9, 2021

View reviewed changes

samouri merged commit 5869aed into ampproject:main Jul 9, 2021

samouri deleted the build-consts2 branch July 9, 2021 17:48

ampprojectbot added PR use: In Beta / Experimental PR use: In Stable labels Jul 13, 2021

samouri mentioned this pull request Jul 21, 2021

🐛 fix build-constant isForTesting #35335

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build-system: consolidate all build time constants #34327

build-system: consolidate all build time constants #34327

samouri commented May 11, 2021 •

edited

Loading

amp-owners-bot bot commented Jul 8, 2021 •

edited

Loading

samouri Jul 8, 2021

rcebulko Jul 8, 2021

rcebulko Jul 8, 2021

rcebulko Jul 8, 2021

samouri Jul 8, 2021

samouri commented Jul 8, 2021

rsimha left a comment

rsimha Jul 9, 2021

samouri Jul 9, 2021 •

edited

Loading

rsimha Jul 9, 2021

samouri Jul 9, 2021

build-system: consolidate all build time constants #34327

build-system: consolidate all build time constants #34327

Conversation

samouri commented May 11, 2021 • edited Loading

amp-owners-bot bot commented Jul 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samouri commented Jul 8, 2021

rsimha left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samouri Jul 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samouri commented May 11, 2021 •

edited

Loading

amp-owners-bot bot commented Jul 8, 2021 •

edited

Loading

samouri Jul 9, 2021 •

edited

Loading