internetarchive / iaux

Monorepo for Archive.org UX development and prototyping.
GNU Affero General Public License v3.0
67 stars 86 forks source link

Define activity indicator in main indexjs #859

Closed iisa closed 1 year ago

iisa commented 1 year ago

Problem:

our esm instance cannot reconcile a sibling file of its main index.js with the same name as the package.

problem file for esm: https://github.com/internetarchive/iaux/blob/master/packages/ia-activity-indicator/ia-activity-indicator.js it thinks it's https://github.com/internetarchive/iaux/blob/master/packages/ia-activity-indicator/index.js

See: https://esm.archive.org/v99/@internetarchive/ia-activity-indicator/ia-activity-indicator.js versus: https://unpkg.com/@internetarchive/ia-activity-indicator@0.0.4/ia-activity-indicator.js

Solution:

define the custom element in the index.js file

dev note: we cannot define in the src/ia-activity-indicator.js file for backwards compatibility.

Testing:

pull down branch, run:

demo loads with activity indicator showing

image
traceypooh commented 1 year ago

Q? wouldn't we normally like the customElements define in the main .js file?

for example, we see this here:

tracey@minti  4:47PM  /Volumes/bff/dev/iaux/packages/ia-activity-indicator
> cat ia-activity-indicator.js 
import { IAActivityIndicator } from './src/ia-activity-indicator.js';

window.customElements.define('ia-activity-indicator', IAActivityIndicator);

but all the code is in ./src/ia-activity-indicator.js';

so i'm curious why this got split this way?

traceypooh commented 1 year ago

code & solve seem acceptable, but the more i look/poke, the more it seems like the

customElements.define('ia-activity-indicator', IAActivityIndicator);

should just be added to the bottom of this file.

iaux/packages/ia-activity-indicator/src/ia-activity-indicator.js
iisa commented 1 year ago

code & solve seem acceptable, but the more i look/poke, the more it seems like the

customElements.define('ia-activity-indicator', IAActivityIndicator);

should just be added to the bottom of this file.

iaux/packages/ia-activity-indicator/src/ia-activity-indicator.js

yes, i agree. once i add it the definition to src/ia-activity-indicator.js -> all the consumers of ./ia-activity-indicator.js will hit the double registry conflict. then i will have to update each consumer just for this patch. this is the legacy snowpackJS pattern we used to use, which is now obsolete. i am just fixing esm's issue, where it cannot find ./ia-activity-indicator.js. 👍

traceypooh commented 1 year ago

ok, it's merged. but am curious for in future, what if we simply wrapped w/ a try/catch to avoid any double includes? eg:

try {
   customElements.define('ia-activity-indicator', IAActivityIndicator);
} catch {}