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

(dev/core#4279) Define import-maps for ECMAScript Modules (OOP) #26309

Merged
merged 21 commits into from
Jun 7, 2023

Conversation

totten
Copy link
Member

@totten totten commented May 23, 2023

Overview

With ECMAScript Modules (ESMs), one *.js file can import another *.js file. The import statement may use a physical-path or a logical-path. Logical-paths are easier to read, more portable, and more maintainable. This branch adds support for logical-paths.

To try it out, install the esmdemo extension and open page civicrm/esmdemo.

(This is the second part of https://lab.civicrm.org/dev/core/-/issues/4279. It builds on #26195. It is an alternative to #26197 which has a different hook signature.)

(Analogy: ESM import-maps serve roughly the same purpose as PHP auto-loaders. They allow you to load other modules without knowing the exact file-path.)

Before

The basic ESM support (#26195) allows you to use import - but only with a physical path.

import { TableWidget } from 'https://example.com/sites/all/modules/civicrm/packages/supertables/table-widget.js';

After

You may import a *.js file with a logical-path like:

import { TableWidget } from 'supertables/table-widget.js';

To do so, you must declare the logical-path as part of the "import map":

function my_civicrm_esmImportMap(\Civi\Esm\ImportMap $importMap) {
  $importMap->addPrefix('supertables/', E::LONG_NAME, 'packages/supertables-1.2.3/');
}

The $importMap is an object. It currently accepts registrations as ext-relative-paths. This allows the contract to be the same while leaving some flexibility in how we implement it; e.g.

Technical Details

In the attached *.md file, you will find discussion about how import-maps should work from a few different perspectives:

  • Extension Developer: How to register additional mappings
  • Core Developer: How to render a default SCRIPT tag
  • UF/CMS Developer: How to integrate Civi's import-map into the UF/CMS import-map.

Comments

This is currently marked draft for a few reasons:

  • I need to write+test an example consumer as part of https://github.com/totten/esmdemo/
  • It needs some phpunit test-coverage.
  • I'd also like to take a stab using the shim for older browsers.
  • After we have some working examples, we may want to talk more about the namespace conventions. (What makes for a good or bad import-map?)

totten added 20 commits May 22, 2023 14:40
(a) I don't really care about customizing `esm_loader` via web UI on a
day-to-day basis. It's not terrible that the web UI has the setting,
but that's not the main point. So why have a setting?

(b) When/if some CMS or CMS-addon causes a break in ESM loading, the
site-builder should be able to swap loaders and work-around the problem.
Neither loader is invicible, but they have some complementary failure-modes.
Compare:

  - If the conflict is two browser-level "importmap"s, then switching
    Civi to "importmap-shim" (`esm.loader.shim`) should work-around that.
  - If the conflict is two copies of "es-module-shims.js", then switching
    to basic "importmap" (`esm.loader.browser`) should work-around that.

(c) I went back/forth on whether the configuration should be a constant
(`CIVICRM_ESM_LOADER`), setting (`esm_loader`), or something even more
esoteric. In the event of a failure/conflict, settings provide the most
media through which the site-builder can enable a work-around, eg

  - Edit `civicrm.settings.php`, or
  - Edit `/etc/civicrm.settings.d`, or
  - Use CLI like `cv vset` or `drush cvapi`
  - Use browser console like `CRM.api3` or `CRM.api4`
  - Use REST API

(d) When/if there's a conflict, the long-term fix will (probably) be
implementation of another loader (`esm.loader.drupal` or `esm.loader.wp`)
based on some future API. At that point, we would need to change the default
for that environment (*without changing defaults for anyone else*). Hence
the support for dynamic defaults (a la `prevNextBackend` or `assetCache`).
These flags were drafted when there was single "loader" class -- the idea
was that a UF might toggle these flags. However:

* The branch now supports swapping the "loader" class entirely.
* It's easier to read the class if you don't have so many flags.
* We don't have a real UF contract to compare against, so the
  flags are a bit speculative.
@civibot
Copy link

civibot bot commented May 23, 2023

(Standard links)

@colemanw
Copy link
Member

Cool @totten - I assume it's on your radar to add a mixin for auto-implementing the esmImportMap hook.

@totten
Copy link
Member Author

totten commented May 26, 2023

We were previously discussing the edge-cases for what happens if a future CMS or CMS-plugin inserts an importmap, so I decided to provoke some conflicts.

I tested with Firefox v113.0.2, Chromium 113.0.5672, and es-module-shims v1.7.2. As predicted, you can provoke conflicts - but they can be resolved with opposite settings, e.g.

If D7's esmconflict uses... Then set Civi to... So that...
browser or shim-fast shim-slow They coexist peacefully.
shim-slow browser or shim-fast They coexist peacefully.

(@colemanw) I assume it's on your radar to add a mixin for auto-implementing the esmImportMap hook.

Yeah, I think a mixin is a good idea. We can do that as a follow-up.

@totten totten marked this pull request as ready for review May 26, 2023 01:23
@colemanw
Copy link
Member

@totten good sleuthing and conflict-provocation. At this stage since all conflicts are theoretical, I'm satisfied simply to know that they all have a theoretical resolution.

@colemanw
Copy link
Member

colemanw commented May 26, 2023

I think this PR is in a good state to merge. It's documented, has tests, and Tim has given it a good bit of kicking including some light CMS warfare. The code looks good to me. Giving merge-ready.

@colemanw colemanw added the merge ready PR will be merged after a few days if there are no objections label May 26, 2023
@totten
Copy link
Member Author

totten commented May 26, 2023

I'd like to recap an issue that was hinted in the comments -- and we've discussed a few times.

  • "The Scoping Issue":
    • In UF land (Drupal/WP/etc), the UI can load a bunch of JS libraries -- jquery, lodash, angular, react, ad nauseum.
    • In Civi land, the UI can load a bunch of JS libraries -- jquery, lodash, angular, react, ad nauseum.
    • But how do you decide which version of each library is used - and at which time? This is the scope of visibility for the specific libraries/versions.
  • Scoping Policies: There are a few high-level approaches to take:
    1. "One Version to Rule Them All": Everything in UF+Civi needs to use the exact same version for every library.
    2. "Let 100 Versions Bloom": Every page/UI/package/file can have a bespoke mix of libraries. There can many copies of the same library.
    3. "Two Separate (Yet Equally Important) Versions": UF+Civi are separate worlds. Each is internally unified.
      • This is the high-level policy we've followed to-date. It seems like a decent balance between maintainability and efficiency. So we'll carry-on with that.
  • Scoping DX: If you are writing an ECMAScript Module that requires lodash, how do you get it? There are a couple designs you might follow. Whichever DX we follow, we should follow it consistently (from the beginning).
    • "Coding Convention (Explicit)": Whenever you want to import CiviCRM's version of a library, say so explicitly.
      import _ from 'civicrm/lodash/lodash.js';
      import _ from 'other/lodash/lodash.js';
    • "Smart Loading (Implicit)": Always import using the canonical name of the library. It is the job the module-loader to figure out which version you want (based on the context or other metadata). This is how most of the JS ecosystem works.
      import _ from 'lodash/lodash.js';
  • Scoping Implementations: Given the high-level policy+DX, how do we get the right files loaded?
    • For "Coding Convention", this starts easy (just include the extra prefix when you register libraries). It breaks down as you share code or documentation with more projects. (For example, you might import civicrm/jquery-superscroll/jquery.superscroll.js -- which then imports jquery/jquery.js -- which is not the version we intended.)
    • For "Smart Loading", it also starts easy. The hard part comes when you actually need multiple copies of the same library.
    • For "Smart Loading", it is theoretically possible to define scopes in the browser-based import-map. However, it's not yet clear how we would map Civi's file-hierarchy into the scopes. It seems to require (a) detailed knowledge of the JS dependency-graph or (b) spamming the "importmap" with excessive info or (c) a different file-hierarchy.
    • For "Smart Loading", there are other options - esp. if you're willing to do some compilation or linking in advance. These are things like:
      • Filter the JS file content. Find import .* from ".*" and modify the path/URL; or...
      • Sync the JS files to another file-tree with a more normalized structure; or..
      • Call-out to an external linker/loader tool.

In discussions with Coleman/Eileen/Seamus, the "Smart Loading (Implicit)" DX seems to be agreed.

import _ from 'lodash/lodash.js';

We can see the implementation evolving in three periods;

  1. Green Pasture Period: We're free to define importmaps and use canonical file-names. No other parties are defining importmaps. Pretty much anything we do is OK. We don't need any special mechanism for "Smart Loading".
  2. Transitional Period: Other parties (CMS's or CMS plugins) gradually start to introduce importmaps. This creates conflicts. We don't know when/if/how this will happen. During this period, some admins may need to address conflicts by setting esm_loader=shim-slow or esm_loader=browser. Swapping these won't be perfect, but it should be decent for a transitional period.
    • (Added) What's imperfect about the transitional period? After all, testing found "Superficial Conflicts" and "Substantive Conflicts" to be OK. But:
      • It requires manual intervention to change the setting.
      • Browser-versions are a wildcard. I expect "Substantive Conflicts" will still afflict old browsers (which lack native importmaps). OTOH, this portion of the market should be shrinking as we speak.
  3. Mature Period: There are 3+ ways that we can plausibly implement a more mature loader. Once we have some more experience+info (from the Green Pasture and Transitional Periods), then we can settle on which design(s) to support in the mature period.

@eileenmcnaughton eileenmcnaughton merged commit fb83688 into civicrm:master Jun 7, 2023
@totten totten deleted the master-esm-oop2 branch June 7, 2023 05:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
master merge ready PR will be merged after a few days if there are no objections
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants