Multiple versions of @embroider/macros may conflict at runtime in classic builds

[rwjblue]

When multiple versions of @embrodier/macros co-exist at runtime (in non-Embroider builds) the @embroider/macros/runtime module will not be guaranteed to be the one that the babel plugin (which injects the import statement) expects. Specifically, this is because the ember-cli build pipeline merges multiple versions of addons down in such a way that it is quite confusing to reason about which one "wins", but it is very likely to be the "wrong one" for at least one consumer.

Specifically, if this code runs:

embroider/packages/macros/src/babel/macros-babel-plugin.ts

Lines 222 to 233 in 45f2a7c

	function addRuntimeImports(path: NodePath<Program>, state: State) {
	if (state.neededRuntimeImports.size > 0) {
	path.node.body.push(
	importDeclaration(
	[...state.neededRuntimeImports].map(([local, imported]) =>
	importSpecifier(identifier(local), identifier(imported))
	),
	stringLiteral(pathToRuntime(path, state))
	)
	);
	}
	}

It will always default to @embroider/macros/runtime:

embroider/packages/macros/src/babel/state.ts

Lines 53 to 55 in 45f2a7c

	// running inside a classic build, so use a classic-compatible runtime
	// specifier
	return '@embroider/macros/runtime';

Which is absolutely not guaranteed to have matching implementations between the babel plugin that emits the import and the actual runtime function that will be found and ran.

---

My suggestion: update pathToRuntime and the packages/macros/src/ember-addon-main.ts to "scope" the addon folder contents under a versioned path (e.g. @embroider/macros/1/runtime) such that the import that is injected is guaranteed to match implementations between babel plugin side of things and the runtime side. I don't anticipate that the version would explicitly match the package.json version (I'd bet most of the time we release a new version of @embroider/macros we don't break the contract between babel plugins and runtime macros) but having a revision to bump when the coupling between the two sides (build time + runtime) will ensure that the two versions of @embroider/macros can co-exist together.

[rwjblue]

FWIW, I am fairly sure that this is not an issue in Embroider builds since we'd fall into this branch:

embroider/packages/macros/src/babel/state.ts

Lines 48 to 51 in 45f2a7c

	if (!state.opts.owningPackageRoot) {
	// running inside embroider, so make a relative path to the module
	let source = sourceFile(path, state);
	return explicitRelative(dirname(source), runtimePath);

And the import path would be relative to the "includer" (and therefore duplicate as needed).

[ef4]

In embroider builds, even that wouldn't come up, because there's only ever one version of @embroider/macros really doing all the work. We actively disable the plugins from any versions that addons brought with them.

[rwjblue]

Gotcha, perfect.

So this definitely will only affect ember-cli build pipeline. I do think it needs to be resolved before broadly suggesting addons migrate to @embroider/macros though.

[simonihmig]

I do think it needs to be resolved before broadly suggesting addons migrate to @embroider/macros though.

I did that though, and yes, there were some rough edges. Loosely related: #509 (and still not quite resolved as apps using ember-auto-import < 1.7.0 will still likely be affected by this)

[ef4]

I agree that we could solve this by versioning the runtime as @rwjblue describes.

But I also think we could solve this the way ember-auto-import does, by choosing one copy to rule them all and letting it do all the real work, in both babel and runtime. And the reason I'd favor that strategy if practical is that it's closer to how the macros work under embroider.

[ef4]

So having reviewed the code and thought about it, I think going with runtime versioning is fine. Because of the constraints of running in the classic mode, it's actually better to let each copy of macros do its own babel on its own consumers.

[ef4]

As far as I can tell, we haven't yet shipped any incompatible releases. The first release that had the runtime code in treeForAddon was 0.20.0, and it hasn't changed since then. So I think this is only a theoretical future problem and not a current one.

I can add documentation to the runtime module to warn against that eventuality, but I don't think any actual code changes are needed right now.

[boris-petrov]

@ef4 - I'm not sure you're talking about the same thing, but @rwjblue opened this issue exactly because there was a case with conflicting versions. Just wanted to mention that if you haven't seen it.

[ef4]

Those versions don’t actually conflict, because only one of them contains addon/runtime.js.

It looks like ember-cli-addon-guard will complain even if we use @rwjblue’s suggested solution here. It doesn’t know whether an addon is being careful to tolerate multiple versions.

[boris-petrov]

@ef4, @rwjblue - I hit this issue yet again with ember-exam and ember-element-helper:

ATTENTION: ember-cli-addon-guard has prevented your application from building!

Please correct the following errors:

Your application is dependent on multiple versions of the following run-time addon:

@embroider/macros
----------------------------------------
frontend
├─┬ ember-classic-decorator
│ └── @embroider/macros@0.36.0 (cacheKey: 62be3924854e06eb5a93d04a03730070, runtime: true)
├─┬ ember-element-helper
│ └─┬ @embroider/util
│   └── @embroider/macros@0.39.1 (cacheKey: e2aaad4047836faa30eba1d9f4252edb, runtime: true)
└─┬ ember-exam
  └── @embroider/macros@0.36.0 (cacheKey: 62be3924854e06eb5a93d04a03730070, runtime: true)

And that's even when I'm using my own branch of ember-classic-decorator as otherwise there is even a third different version of @embroider/macros.

What am I to do with all this? If every single addon uses their own version of @embroider/macros how is this going to scale?

[ef4]

Addon guard is wrong, there is no problem with your setup. There is no correctness problem (because all versions of macros have compatible runtimes), and there is no wasted bytes problems (because ember-cli will collapse the runtimes together into a single copy).

If every single addon uses their own version of @embroider/macros how is this going to scale?

First, in the app it scales just fine for the reasons above.

Second, addons having a dependency on @embroider/macros is only a compatibility bridge. V2-formatted addons can't bring their own copy of the macros, and that is what we're moving the ecosystem toward.

[rwjblue]

We should update ember-cli-addon-guard to make it aware that this package is safe, but you can add @embroider/macros to your addon guard config's ignoreAddons for now.

[boris-petrov]

Ah, OK, thank you both, I didn't know the ignoreAddons config option. That works for me, thanks!

@rwjblue - if you open an issue or something in ember-cli-addon-guard, please ping me so I can subscribe. Thanks!

[tehhowch]

I ran into an issue with @embroider/macros in an ember-cli build (ember 3.24 w/edition=octane) once adding ember-classic-decorator into the mix

Build Error (broccoli-persistent-filter:Babel > [Babel: @embroider/macros]) in @embroider/macros/runtime.js
[BABEL]: Module did not self-register: '/path/to/monorepo/node_modules/canvas/build/Release/canvas.node'. (While processing: /path/to/monorepo/packages/my-app/node_modules/@embroider/macros/src/babel/macros-babel-plugin.js)

the versions that yarn installed were 0.29.0 (via ember-classic-decorator v2.0.1), 0.33.0 (ember-auto-import v1.10.1), and 0.41.0 (ember-basic-dropdown via ember-power-select v4.1.6)

pinning @embroider/macros to 0.41.0 resolved the build issue (for now at least).

[ef4]

@tehhowch I don't think that's the same as the above issue, please file a new one if you have a reproducible problem.

[jking6884]

Would this potentially be related to this issue? I'm seeing this after installing certain addons.

I'm seeing this when running yarn start after installing ember-bootstrap@4.9.0

[ef4]

Yes, that looks like this bug.

[lifeart]

Related issues:

• 4.0.4 build error on Ember 3.28.8 cibernox/ember-basic-dropdown#656
• Allow Embroider v1 deps; Drop node v10 tildeio/ember-element-helper#74 (comment)
• @babel/template placeholder build error resurrection #1077

[meirish]

I saw the same error mentioned here today (with a different placeholder) when attempting to upgrade ember-cli-mirage. There's a related issue here: miragejs/ember-cli-mirage#2343