embroider-build / embroider

Compiling Ember apps into spec-compliant, modern Javascript.
MIT License
333 stars 136 forks source link

`each()` macro doesn't unroll in development builds #1063

Open Panman82 opened 2 years ago

Panman82 commented 2 years ago

TLDR: It should (and does in production), remove the state.opts.mode checks and always unroll.


In discussion over on ember-auto-import, it seems that the each() macro doesn't unroll for development (run-time mode) builds.

https://github.com/embroider-build/embroider/blob/57d58a7e3c0dbfece76c9ee2b5136622105d0a5a/packages/macros/src/babel/each.ts#L52-L54

This will cause "(template) string literal" requirement errors when using the macro as shown in the RFC example.

// This example:
import { getOwnConfig, each, importSync } from "@ember/macros";
let plugins = [];
for (let plugin of each(getOwnConfig().registeredPlugins)) {
  plugins.push(importSync(plugin).default);
}

// could compile to this, given OwnConfig
// containing { registeredPlugins: ['@bigco/bar-chart', '@bigco/line-chart']}

let plugins = [];
plugins.push(importSync("@bigco/bar-chart").default);
plugins.push(importSync("@bigco/line-chart").default);
Build Error (broccoli-persistent-filter:Babel > [Babel: addon-name]) in addon-name/components/foobar.js

/path/to/addon-name/components/foobar.js: import() is only allowed to contain string literals or template string literals
  1 | // This example:
  2 | import { getOwnConfig, each, importSync } from "@ember/macros";
  3 | let plugins = [];
  4 | for (let plugin of each(getOwnConfig().registeredPlugins)) {
> 5 |   plugins.push(importSync(plugin).default);
    |                ^^^^^^^^^^^^^^^^^^
  6 | }

This happens because ember-auto-import imposes the dynamic import() requirements for the importSync() macro here.

https://github.com/ef4/ember-auto-import/blob/d7031b95168c7f280d8be74d08a56e375b965104/packages/ember-auto-import/ts/analyzer-plugin.ts#L55-L57

        } else if (callee.isIdentifier() && callee.referencesImport('@embroider/macros', 'importSync')) {
          state.imports.push(processImportCallExpression(path, false));
          state.handled.add(path.node);
        }

However, it will compile successfully in production builds, when the each() is unrolled (as shown in the example).

https://github.com/embroider-build/embroider/blob/57d58a7e3c0dbfece76c9ee2b5136622105d0a5a/packages/macros/src/babel/each.ts#L56-L62

@ef4 says the best solution is to have the each() macro (and macroCondition()) look at argument information instead of build mode when deciding their result. The argument information could be added to the EvaluateResult, and subsequently used by the macros when needed.

https://github.com/embroider-build/embroider/blob/6a2e220f08db8558af8366e9d186d2f5df86953f/packages/macros/src/babel/evaluate-json.ts#L103-L106

But I'm not really sure what additional argument information is needed and how to get that information. I feel like all the needed checks are already done.

https://github.com/embroider-build/embroider/blob/57d58a7e3c0dbfece76c9ee2b5136622105d0a5a/packages/macros/src/babel/each.ts#L42-L50

So the state.opts.mode checks can just be removed, leaving just the unroll stuff below the confident/array checks. 🤷‍♂️

Panman82 commented 2 years ago

I see where the different scenarios are being tested, but I still don't understand why run-time vs build-time even matters... 🤔

https://github.com/embroider-build/embroider/blob/cd619f370b060457419e5bde58ca156b42bf29c4/packages/macros/tests/babel/each.test.ts#L23-L49