glimmerjs / glimmer.js

Central repository for the Glimmer.js project
https://glimmerjs.com
MIT License
746 stars 75 forks source link

1.x emits invalid ES modules on Ember >= 3.20-beta.4 #316

Closed ef4 closed 3 years ago

ef4 commented 3 years ago

embroider-compat-audit flagged the behavior implemented in https://github.com/glimmerjs/glimmer.js/pull/293 as a problem under ember >= 3.20.0-beta.4, because ember-component-manager.ts imports setDestroyed and setDestroying from destroyables.ts, but the overridden version of destroyables.ts doesn't export those names.

(This is consistent with the ES module spec, which considers importing a non-exported name a SyntaxError.)

It seems like it would be better to always present a consistent interface out of destroyables.ts, instead of having ember-component-manager.ts do its own separate gte('3.20.0-beta.4').

Alternatively, to keep things organized the same, we could switch to:

import * as destroyables from './destroyables';
const { setDestroyed, setDestroying } = destroyables;

which is legal because it always imports the whole module namespace and then inspect it at runtime, where it may find that these names are undefined and that's OK. For searchability, the audit failure looks like:

Screen Shot 2020-11-04 at 12 51 40 PM
./node_modules/@glimmer/component/-private/ember-component-manager.js
  importing a non-existent named export: "./destroyables" has no export named "setDestroyed".
       6 | import { schedule } from '@ember/runloop';
       7 | import BaseComponentManager from './base-component-manager';
    >  8 | import { setDestroyed, setDestroying } from './destroyables';
         |         ^^^^^^^^^^^^
       9 | const CAPABILITIES = true // @ts-ignore
      10 | // @ts-ignore
      11 | ? capabilities('3.13', {
  importing a non-existent named export: "./destroyables" has no export named "setDestroying".
       6 | import { schedule } from '@ember/runloop';
       7 | import BaseComponentManager from './base-component-manager';
    >  8 | import { setDestroyed, setDestroying } from './destroyables';
         |                       ^^^^^^^^^^^^^
       9 | const CAPABILITIES = true // @ts-ignore
      10 | // @ts-ignore
      11 | ? capabilities('3.13', {
  file was included because:
    ./node_modules/@glimmer/component/index.js
    ./components/addons/ember-animated.js
    ./tests/integration/components/addons/ember-animated-test.js
    ./assets/test.js
    ./tests/index.html
    packageJSON.ember-addon.assets

This problem doesn't exist on master, should we bother doing a patch release to the 1.x series? I'm fine with WONTFIX, I just wanted to file this so there's a place to point people when they see this audit failure.

rwjblue commented 3 years ago

Fixed by https://github.com/glimmerjs/glimmer.js/pull/331 (included in 1.0.4)