-
Notifications
You must be signed in to change notification settings - Fork 4.1k
Description
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
devAssertare easily resolved with the pure assertion helpers insrc/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/preactcan import fromsrc/sharedorsrc/preactwithout 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
- Anything in this directory can import from
- We don't want to duplicate the src-dependency-web
- Promote small helper modules with clear functions
- Create
shareddirectory requiring explicit add-in, instead of "opt-out"runtimedirectory - 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
- Disallow any Bento or Preact code from importing non-shared
TL;DR - Directory of helpers that can be included with Preact/Bento packages and have nothing to do with AMP
Steps
- Create
/src/shareddirectory - Write ESLint rule to assert file X imports only from
/src/sharedand/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
pureFooBarhelpers- 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