I2I: src/core directory with no runtime dependencies

[rcebulko]

Overview

The main amphtml/src directory holds over 100 JS files (almost 300 if you count subdirectory contents). Some of these files are isolated utilities like date parsing, string manipulations, etc. Others seem like small utilities, but may actually depend on large chunks of the runtime and a whole web of dependencies

Example of issue: At the time of this writing, src/preact/base-element imports devAssert, which in turn initializes logging constructors, two different loggers, binds a global error handler, pulls in the AMP config and runtime version, and getMode helpers reading from AMP global variables. Suddenly, because of this one import, every Bento component depends directly on all of this AMP boilerplate. This is an extremely easy oversight for any developer.
Edit: There are other dependencies of src/preact/base-element that have transitive dependencies on assertions/logging; it is not just this import

Note: This example and others relying on devAssert are easily resolved with the pure assertion helpers in src/pure-assert.js, but the issue remains.

AMP components can rely on and use any of these files. Preact/Bento components cannot, or they'll pull in whole chunks of the AMP dependency tree. As more Bento components are written, more shared helpers are needed, and it becomes harder to pull only "pure" helpers.

I propose creating a dedicated directory (ex. amphtml/src/shared) for these files. New shared helpers would go here. Existing helpers could be moved gradually as they are needed. ESLint rules could be added to block importing from non-shared modules so devs get a warning at dev-time rather than by seeing the bundle size after compilation.

Suggested Approach

• Dedicated src/shared/ directory
  • Anything in this directory can import from ./* and remain shared
  • Anything in src/preact can import from src/shared or src/preact without pulling in AMP dependencies
  • AMP and Bento code can import from ./shared, including runtime helpers
  • This directory can become the Bento/Preact shared module on NPM
• We don't want to duplicate the src-dependency-web
  • Promote small helper modules with clear functions
  • Create shared directory requiring explicit add-in, instead of "opt-out" runtime directory
  • No bulk-moving existing helpers into shared; only move helpers as they are used
• ESLint Rule
  • Disallow any Bento or Preact code from importing non-shared src/ files
  • If possible, disallow anything with side effects

TL;DR - Directory of helpers that can be included with Preact/Bento packages and have nothing to do with AMP

Steps

• Create /src/shared directory
• Write ESLint rule to assert file X imports only from /src/shared and /src/preact
  • Applies to all files in both of those directories
• Migrate existing helpers that are already used in Preact into /src/shared
• Expand support for linter to include opted-in component files
• Write or find existing ESLint rule to assert shared files are idempotent
  • Until then: diligent OWNERS during review

AlternativesConsidered

• Have a bunch of pureFooBar helpers
  • Pro: usage is clear and visible inline
  • Con: somewhat ugly and verbose, very much AMP-centric not Bento-centric
• No structural changes; just be careful about imports + raise compilation error
  • Pro: minimal code changes
  • Con: error-prone; developers don't catch dependencies until bundle-size time, if at all

/cc @ampproject/wg-bento @ampproject/wg-performance

[rcebulko]

Related to #27408

[caroqliu]

Bikeshed: src/core, src/standalone, or src/plain? src/shared is a bit confusing since the concept of "sharing" is relative to its use, which is only given meaning by already knowing how it's being shared.

In particular want to avoid src/pure (and would recommend even renaming pure-assert) due to its loaded social connotations, i.e. with the social purity movement and such.

[rcebulko]

In particular want to avoid src/pure (and would recommend even renaming pure-assert) due to its loaded social connotations, i.e. with the social purity movement and such.

Absolutely agree

[dvoytenko]

And another food for thought: are we going to dupe devAssert and similar utilities? Or will we just immediately try to make them "pure" for all existing code? TBH, I feel like it'd be nice to go for the wholesome replacement right away or very soon.

[rcebulko]

+1 to simplifying devAssert and userAssert throughout. Will look into it and see if they rely heavily on the logging bits or if they can be stripped down and log.js can depend on them seprately

[dvoytenko]

I think we should just go ahead and break the dependency with log.js asap.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.