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 #26197

Closed
wants to merge 17 commits into from

Conversation

totten
Copy link
Member

@totten totten commented May 11, 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.)

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/js/table-widget.js';

After

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

import { TableWidget } from 'civicrm/js/tab-widget.js';

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

function my_civicrm_esmImportMap(array &$importMap, array $context) {
  $importMap['imports']['civicrm/'] = Civi::paths()->getUrl('[civicrm.root]/.');
}

The structure of $importMap is defined by the web-browsers. See, e..g.

Technical Details

In the docblocks, 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?)

@civibot
Copy link

civibot bot commented May 11, 2023

(Standard links)

@civibot civibot bot added the master label May 11, 2023
@totten
Copy link
Member Author

totten commented May 11, 2023

Thanks @eileenmcnaughton. I pushed one commit for all three suggestions.

@totten
Copy link
Member Author

totten commented May 11, 2023

totten added 11 commits May 11, 2023 17:20
(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`).
@totten
Copy link
Member Author

totten commented May 16, 2023

@eileenmcnaughton @colemanw - Since importmap only recently hit the milestone of cross-browser support (https://caniuse.com/?search=importmap), I played around with a couple mechanisms to load import-maps on older browsers. Most notably:

The next question: looking down the road, what happens if importmap is used in a future version of Drupal or WordPress or Joomla or Backdrop or one of their many addons? And does it make a difference if Civi (or the counterparty) uses es-module-shims? I believe there are a few scenarios for conflicts.

But I think I found a neat way to mitigate the impact of conflicts: make es-module-shims optional. (Admins can set esm_loader=shim or esm_loader=browser). The key thing is that you can force it into shim-mode (where shimmed-modules are separate from browser-native modules). So:

  • If (e.g.) WP adopts es-module-shims for importmap, then Civi-WP users can set esm_loader=browser to temporarily side-step a conflict.
  • If (e.g.) Drupal adopts browser-native importmap, then Civi-Drupal users can set esm_loader=shim to temporarily side-step a conflict.

Those aren't perfect solutions. (They may require you to trade performance or compatibility; and they're not guaranteed to work in every hypothetical conflict.) But it's a fairly simple adaptation and which plausibly mitigates several contingencies.

So the current revision adds the setting. I also added a Markdown doc discuss more about this.

@colemanw
Copy link
Member

@totten

The key thing is that you can force it into shim-mode (where shimmed-modules are separate from browser-native modules)

How would you do that? ES Module Shims seems to act like a polyfill that only kicks in if the browser lacks support for import maps. From the GitHub page:

ES Module Shims entirely bypasses processing for the 74% of users with native import maps support.

Maybe deeper down in the docs is something about forcing it to override the native browser support?

@totten
Copy link
Member Author

totten commented May 16, 2023

(@colemanw) Maybe deeper down in the docs is something about forcing it to override the native browser support?

That's correct. It responds to a few different tags:

<script type="importmap">...</script> <!-- Dynamic polyfill. Use browser-native-mode or shim-mode -->
<script type="importmap-shim">...</script> <!-- Use shim-mode -->

And there's also this clarification. (Note: The README tends to use "polyfill" and "shim" interchangeably.)

... [There] are effectively two loaders running on the page ... if you have two module graphs - one native and one polyfilled, they will not share the same dependency instance...

--

Relatedly - the current revision doesn't enable dynamic polyfill mode, but I wasn't sure about that. Dynamic polyfill mode is probably better balance of compatibility+performance for the typical deployment. (It's just not as useful as an escape-vector in case of conflict.) The only real downside is a longer list of options. ("Browser-native", "Shim-Adaptive", "Shim-Forced").

@colemanw
Copy link
Member

The only real downside is a longer list of options. ("Browser-native", "Shim-Adaptive", "Shim-Forced").

IMO the biggest downside to this approach is giving the admin One More Damn Thing To Configure ™; and the only reason they'd ever want to think about it is if their site is broken due to import map conflicts, and in that case, I think chances are very good that the error message they get (if any) would be so obtuse (maybe a 401 buried in the console somewhere) and so weirdly different on different browsers, that they'd never be able to figure out what was going on well enough to know that it had anything to do with this Damn ™ setting.

(Yes that paragraph was a single sentence. I'm gonna trademark that as well.)

So in case that rant wasn't clear, IMO the ideal solution would not involve the admin configuring an obscure setting after tracking down an obtuse error. Ideally we'd be approaching this the way we siloed off jQuery and LoDash with noConflict() mode. Obviously, having duplicate copies of jQuery isn't the most performant choice for every site, but the admin doesn't have to configure anything and it works for every site.

@totten
Copy link
Member Author

totten commented May 16, 2023

@colemanw I definitely share the fear of giving too many options, but I think it's a misplaced here. Let's contrast a few situations:

  • When extensions were first added to CiviCRM, there was no default path. Everyone had to navigate to two different settings pages to configure the "Path" and "URL". This was bad. It required extra setup on every deployment (and it was rather unforgiving about the details).
  • When writing a builder UI (like "Advanced Search" or "Search Kit" or "Edit Profile" or "New Form"), you have this tension - you can add more functionality to the UX. That requires more widgets. But if you add more widgets, then it becomes more complex for average use-cases.
  • "Administer > System Settings > Misc" is a different beast from either of those. It's a list of things that don't matter 95% of the time. They're not important enough to show-off in some place visible. Most people never think about them. But you can't quite remove the 5%. (Or rather, all options for addressing the 5% are bad, and a setting is less-bad in some ways.)

In drafting, I've gone through a progression from:

  1. I don't want this to be configurable at all. I just want one thing.
  2. Aw shucks, I can't get away with just one thing because of the 5%.
  3. Maybe if I sit on my hands for a day it'll go away.
  4. Nope, 5% still there. Let's sneak in a define()/constant().
  5. Aw shucks, define()s/constant()s aren't as good as settings.

totten added 2 commits May 17, 2023 11:31
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.
@totten
Copy link
Member Author

totten commented May 23, 2023

Hook signature changed. That's a fairly substantive effect on the description, so I refiled as #26309. Closing since that variant seems preferable.

@totten totten closed this May 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants