v2 Addon Format (Embroider compatibility)

[ef4]

Rendered.

[NullVoxPopuli]

is it going to be flexible enough to handle a structure change? like maybe future src top level directory for addons and apps?

[jenweber]

Some additional points to cover in this section:

• Where to document this system (CLI guides)
• How to divide up the learning: general overveiw, change tutorial for new addons, write a tutorial for addon authors to help them migrate their addons, a tutorial for configuration that app users can reference if they run into problems with their addons, and abilities that unlock by having webpack compat. We would also need to revise some existing materials for regular ember-cli builds so that we appropriately present both options. (side note, what would we actually recommend as the happy path).

[jenweber]

This section would benefit from a link to an existing, Embroidered Ember app or two (Octane + Super Rentals) and an addon (???). The necessary detail in this RFC makes it sound very complicated to use Embroider, while the median experience is pretty straightforward.

[ef4]

is it going to be flexible enough to handle a structure change? like maybe future src top level directory for addons and apps?

There are two levels of indirection that make it flexible.

One is that this is a publication format, not an authoring format. But I don't think that's a totally satisfying answer, because there will be a natural desire over time to make the two conform more closely.

The other indirection is that no directories in this format are hard-coded. For example, to use a feature like App Javascript, there's no special directory. Instead there's a property in package.json (ember-addon.app-js) that you can point at any directory in the package.

But there is a hard rule here: whatever paths you publish to NPM in your package, those are the actual paths your users can import from. That is an area where too much flexibility has been bad for us, and we need to follow a clear standard.

[buschtoens]

I recognize that we want to keep devDependencies as an allowed dependency for apps, since this is how it's always been conventionally for Ember apps and we want to reduce churn as much as possible.

Out of curiosity I would still like to understand, why it ever was this way. Is it just a side-effect of the fact that apps are always the root of any given project and thus will always have access to devDependencies?

I would argue that, while still officially supporting it, we should discourage adding runtime dependencies for apps to devDependencies instead of dependencies, for the following reasons:

• It is confusing for Ember newbies.
• It is inconsistent.
• When I turn an app into an engine (an addon) or extract certain parts of an app into an addon (or engine), devDependencies suddenly turn into dependencies, just because the code was moved.

Instead, I would recommend that we encourage the same mental model for apps and addons. Everything that needs to be available at runtime / should influence the production build (an allowed dependency), has to be a dependency. Everything else, like testing or development support, linters, etc. are devDependencies.

What is your opinion regarding this?

If you agree, we could have an opt-in flag to enforce the same semantics for apps and addons.

[chriskrycho]

I strongly agree with this but would go a step further: if at all possible, I would very much like to see us get to a world where devDependencies and dependencies mean what they sound like: the latter is genuinely runtime-only, the former genuinely development- and build-time only.

It’s possible there are reasons that cannot be the case, in which case I’d love clarification there, because my understanding of the three-phase build pipeline for Embroider suggests that precisely that distinction should be possible. But I very well may be missing something!

[ef4]

Yes, for addons the distinction gets quite strong under this RFC. And it will reduce the number of packages needed by apps (count how many copies of ember-cli-htmlbars and ember-cli-babel you have).

For apps, I agree with:

we should discourage adding runtime dependencies for apps to devDependencies instead of dependencies

But most apps don't have any runtime dependencies. If you do ember b, and then rm -rf node_modules, and then serve dist to a browser, your app still runs. There are no runtime dependencies. Your addons may be used at runtime, but the actual addon NPM packages are only needed during the build.

The exception is Fastboot. Fastboot does imply having some runtime dependencies. Using dependencies for this would simplify and improve fastboot, which today tries to generate a whole separate package.json.

[jelhan]

This is still a little unclear to me. While working on a macro-based build system for Ember Bootstrap to handle Bootstrap version-specific builds I faced some open questions:

• Can another template helper be used as an argument for macroCondition as long as the value could be determined statically? E.g. {{if (macroCondition (and (macroGetOwnConfig "isFoo") (macroGetOwnConfig "isBar")))}}
• Are multiple positional arguments supported by macroCondition? E.g. {{if (macroCondition (macroGetOwnConfig "isFoo") (macroGetOwnConfig "isBar"))}}.
  • If it's not supported, a build time error should be thrown in my opinion.
  • If it's supported, are they combined by AND or by OR?

Using macroCondition together with other template helpers would allow domain-specific macros without requiring an AST transform. For example in Ember Bootstrap we could use something like:

{{#if (macroCondition (has-bootstrap-version 3))}}
  {{! Bootstrap 3 only }}
{{/if}}

With a {{has-bootstrap-version}} template helper like this:

import { helper } from "@ember/component/helper";
import { getConfig } from "@ember/macros"; 

function hasBootstrapVersion([version]) {
  return getOwnConfig('version') === version;
}

export default helper(hasBootstrapVersion);

I don't see such a strong use case for support of multiple conditions. It would require all arguments to be statically determinable and in that case they could also be combined beforehand. So instead of {{macroCondition (getOwnConfig "isFoo") (getOwnConfig "isBar)}} a user could just add a isFooAndBar value to the config. If other template helpers are allowed as arguments even an {{and}} or {{or}} template helper could be used. But I nevertheless see the need to clarify that point.

[ef4]

An arbitrary helper like (has-bootstrap-version 3) isn't going to work because we're not going to be able to run it at build time. But the intention is that you would instead put whatever code you want to run at build time in your configure build hook:

configure() {
  let hasBootstrap3 = figureThisOutHowever();
  return { hasBootstrap3 };
}

And use the macro like:

{{#if (macroCondition (macroGetOwnConfig "hasBootstrap3"))}}
  <UseSomeBootstrap3 />
{{/if}}

[ef4]

As for boolean combinations, if Ember standardizes and, or, and not helpers I think it would be simple to support them here. But that is not anticipated in this RFC.

(The equivalent JS macro does support boolean operators.)

[simonihmig]

The Handlebars macro macroGetConfig is not mentioned in the RFC, I assume by mistake!?

[simonihmig]

Suggested change

	{{#if (macroCondition (dependencySatisfies "some-peer-dep" "^3.0.0")) }}
	{{#if (macroCondition (macroDependencySatisfies "some-peer-dep" "^3.0.0")) }}

[simonihmig]

Suggested change

	<div {{macroMaybeAttrs (getConfig "ember-test-selectors" "enabled") data-test-target=@id }} />
	<div {{macroMaybeAttrs (macroGetConfig "ember-test-selectors" "enabled") data-test-target=@id }} />

[Gaurav0]

It looks like Ember Twiddle can continue to interpret v2 addons with ember-cli as regular addons for now? Also @ef4 at some point I will need your guidance on how to giftwrap addons with embroider (hopefully before any embroider specific addons start to become available).

Here is the code currently being used to giftwrap addons for Ember Twiddle today: https://github.com/joostdevries/twiddle-backend/blob/36b63b1021d22b974d534f01f3c723e50c06a1c7/addon-build-configs/3.18.0/ember-cli-build.js

[kellyselden]

Sorry for the late review. It's a behemoth! Thank you Ed for your tireless work on this.

[kellyselden]

"less dynamic that what we may want" -> "less dynamic than what we may want"

[kellyselden]

That RFC doesn't mention how one would consume such an addon. What happens if you try to import from a missing dep?

[ef4]

import from a missing dep would be a static build error.

To actually import things out of an optional dependency you'd need to guard it with the macro system:

if (macroCondition(dependencySatisfies("the-optional-thing", "*"))) {
  let thing = importSync("the-optional-thing");
  doSomethingWith(thing);
}

[kellyselden]

What is the reasoning of this decision? Is it using esm for broad node support?

[ef4]

It's authored as ES for the same reason that all other code in ember apps and addons is authored as ES. That's the only format that's part of ECMAScript standard.

How it actually runs is an implementation detail (which of course is already true of all the other ES modules in an Ember package). We could certainly use esm or even babel-to-CJS.

[kellyselden]

Are you debating failing the build when a peer dep isn't satisfied? I always imagine the following scenario:

package A has a peer dep on package B
package C has a direct dep on package B
App includes package A, then indirectly satisdfies the peer dep by including package C (on purpose, package C is a bundling package)

npm will warn like crazy, even though the app consumer may know what they are doing. I wouldn't want a strict implementation of peer deps to fail in this case.

[ef4]

This RFC doesn't fail the build for a missing peerDep.

But if you actually try to resolve a peerDep within your Ember code, Embroider implements that resolution strictly. You won't find an indirectly satisfied peerDep because that's just not safe to rely on. It happens to work often based on hoisting, but it also fails sometimes, and I have seen it fail many times in the wild.

Plus, you'll have a bad time adopting Yarn PnP or some other future more optimizable package system if you are relying on this kind of implicit dependencies.

So embroider is strict. A v2 package can't resolve things that aren't allowed dependencies, as defined in the definitions section.

The solution to your example is that package C should have a peerDep on B, so that B can be added once to the app and shared by both A and C (and the app).

"bundling package" is a common pattern today in Ember addons, but for historical and no-longer-true-reasons. Now that importing libraries directly from NPM is easy, bundling packages either go away (if they were only bundling) or become separate libraries (if they were providing some ember-specific things like components and helpers that work nicely with the library).

[kellyselden]

Out of curiousity. If everyone seriously objected to one or more of these following RFCs, would it disrupt the whole system? And leave the implementation of this RFC in a difficult spot? Or can this RFC exist on its own without one of these pieces?

[ef4]

It can exist by itself. In practice it would mean that v1 and v2 packages live side-by-side indefinitely because v2 never finishes implementing all the things v1 packages can do.

[kellyselden]

Perhaps this needs a Drawbacks section? One thing that comes to mind is my comment on the prebuilt assets RFC ember-cli/rfcs#118 (comment). Furthering on that, what is the workflow for symlinking your addon into your app as part of your development flow. Does making a change in your addon get picked up automatically by your app's running live reload server, or do you need a second live reload system to generate the addon's dist?

[ef4]

This RFC is careful not to saying anything about authoring format. So the answer is, "it depends".

It's possible to author addons directly in the format laid out here, in which case there's no build step at all. In practice, I do expect addon authors to want more creature comforts, implying a build step, but how that build tooling works now becomes something it's safe to experiment on without breaking any compatibility.

I intend this RFC to be similar to the level of the Component Manager RFC -- it establishes the baseline primitive that we can commit to supporting, which then frees people to experiment on top of the primitive to figure out the best developer experience.

[ef4]

@simonihmig:

do all addons in dependencies also have to be in v2 format?

No, that would indeed be bad. Each package can update to v2 independently. The embroider build's first stage compiles away those differences.

I am thinking of a concrete example actually: an addon uses a newer Ember feature, that requires a polyfill for older Ember versions. Say in-element (could be anything else that requires an AST transform, like ABI). So it has ember-in-element-polyfill as a dependency, which is still a v1 addon, so relies on a build-time AST transform (basically doing {{#in-element ...}} -> {{#-in-element ...}} when the polyfill is needed).

That would indeed be a problem, but not because ember-in-element-polyfill is a v1 package. It's a problem because the transform is running on the wrong input (it's running based on the version of ember in your addon's dummy app, not the version of ember in the addon's consumers' apps).

v2 packages will need to use polyfills that are updated to work safely with embroider. For example, the in-element polyfill can compile{{#in-element}}... o

{{#if (macroCondition (dependency-satisfies "ember-source" ">=4.0.0")) }}
  {{#in-element}}...
{{else}}
  {{#-in-element}}...
{{/if}}

This would make the template bigger, but only on NPM. Once that template is compiled by the consuming app, it will only contain the correct branch for the app's ember version.

[ef4]

It looks like Ember Twiddle can continue to interpret v2 addons with ember-cli as regular addons for now?

Yes, that would work. We really really do need to make sure everybody publishing v2 addons includes the tooling to make them work correctly within non-embroider builds too. We don't want to split the ecosystem.

This RFC is a step toward making the builds way simpler and that will help twiddle, but before we can really get to that point we need to get past the need for all the compatibility stuff. Once everything is natively v2, and everybody is using strict mode templates, etc, a lot of the build tooling can just go away.

[ef4]

At this point I have responded to the new feedback since FCP and incorporated several good suggestions to clarify the document.

Framework core discussed on our call today and agree FCP should continue this coming week so everyone has a chance to respond to the latest round.

[simonihmig]

v2 packages will need to use polyfills that are updated to work safely with embroider. For example, the in-element polyfill can compile{{#in-element}}...

Yes, understood. This is basically the same transform I posted above. In the same comment I asked this:

Or would the polyfill also have to be rewritten to be somehow v2 aware (it's a different case though as it does not provide any importable modules by itself), so basically

• have some means to plug into the prepublish build of the v2 addon (any defined hooks / conventions for this?)
• do a very different AST transform, something like from: {{#in-element ...}} to:

And that's still not entirely clear to me. So when the polyfill does the transformation we agree upon, it needs to do so at prepublish time, right? And if so, we currently have no infrastructure (APIs, convention) to allow the polyfill to somehow hook into the build process of the addon, before it gets published. So this needs to be addressed somehow, right? (Not necessarily by this RFC, but by something, like an addon build support library)

[ef4]

Not necessarily by this RFC, but by something, like an addon build support library

Right, I've added text proposing this package is called @embroider/addon and that it gives addon authors the tooling they need.

I don't think polyfills will need much new infrastructure. A key thing to realize is that they can just always emit macros, they don't need to know whether they are running at publish time or app build time. The decision to evaluate the macros or not is delegated to the macro system so the polyfill doesn't need to think about it.

[igorT]

A question to explore in a future rfc is whether it is enough to expose global flags like env being prod or dev to a macro that would look something like getConfig('app') or we would also want the configure method to have access to it as well

[ef4]

Agreed. It seems likely we will add a way for the macro system to talk about global things that are truly global. As I was working on fastboot compatibility last week the convenience of that became apparent.

[rwjblue]

We discussed this in todays Ember core team meeting, and are still quite excited to move forward here!

Lets do it!!!

[NullVoxPopuli]

in v2 addons, do we have a way of writing tests?
or do we need to establish a blueprint for all that boilerplate still?

in both glimmer-apollo and ember-welcome-page, neither addon has its own tests -- is this intentional? (did I miss something in the RFC?)
I see that both have test-app, but do we need a test app for unit and rendering tests? I imagine rendering tests can be done without the whole application stack?

[ef4]

That question is beyond the scope of this RFC, because you're free to write tests however you want as long as you publish to NPM in this RFC's format, but I agree we also want to establish good patterns here.

in both glimmer-apollo and ember-welcome-page, neither addon has its own tests -- is this intentional?

Just because the tests live within a test harness application doesn't make them any less the addon's own tests. IMO it makes things more transparent and understandable. The classic pattern of putting /tests outside the /tests/dummy app was a source of unnecessary confusion and not a small amount of pain in @embroider/compat, because it doesn't align the code with the actual module namespacing (./tests/components/foo.js looks like it should be the your-addon/tests/components/foo.js module, but it's actually the dummy/tests/components/foo.js module).

but do we need a test app for unit and rendering tests? I imagine rendering tests can be done without the whole application stack?

That's a whole lot of effort to get a strictly less-capable and less-representative test suite that would require a whole new set of learning materials. Whereas working within a fully-normal Ember app means that anyone who learns how to write tests for their app already knows how to write tests for an addon too.

I realize that heavily-mocked component tests (that often don't even run in a real browser) are popular in some parts of the JS ecosystem, but IMO that stems from pretty deep confusion about the point of tests.

[NullVoxPopuli]

beyond the scope of this RFC

Makes sense -- I didn't know where else to ask though. Are these types of questions better for the forums? Or somewhere else? I feel like it's a bit of uncharted territory because we need to figure out / discuss things that are fundamental to how addons will be built, and since module unification (may it rest in peace), I don't recall the last time that happened 🤔

putting /tests outside the /tests/dummy app was a source of unnecessary confusion and not a small amount of pain

I believe it -- for the people I've worked with making their first addons, this comes up regularly

I realize that heavily-mocked component tests (that often don't even run in a real browser)

oh I wasn't suggesting that, I still think those sorts of tests are terrible. I only imagine that it's doable (conceptually, maybe not technically (yet)), to have setupRenderingTest and setupTest either start their own empty shell of an app, or we provide start-app.js a la ye olde times. For example, @lifeart has this set of scripts to demonstrate starting an app up without ember-cli.

re: the test-app, one of the main things I want to figure out is how to best structure the addon and test-app relationship.
For example, in a monorepo using yarn workspaces, you'd have just one work space per addon -- but now you'd need two.
It's not the end of the world for sure, but is a bit of an ergonomics / migration hurdle, as, we need to properly name the test-app, and set it to private in package.json (again not a big deal, just "a thing" if you know what I mean).
atm, I'm thinking:

# previously
package.json (root of workspaces)
apps/
  ...
packages/
  {addon-name}/
     package.json
     addon/
     tests/
     
# with addon-v2     
apps/
  ...
packages/
  {addon-name}/
    package.json (separate internal workspace root?)
    addon/
      package.json (roughly similar to previous {addon-name}/package.json)
      src/
    test-app/
      package.json
      tests/

[ef4]

Yes, if you already have a monorepo making the test-app and the addon separate workspaces is very straightforward. You can organize them however makes sense to you (except that yarn doesn't allow nesting of workspaces, so one cannot be the child of the other).

For people who don't want a monorepo, a small amount of special handling is required to get the linking and dependency management correct. That is the approach that I took in ember-welcome-page, where it uses some utilities provided by @embroider/addon-shim (which I'm about to move into a dedicate @embroider/addon-dev package instead, since they're really only dev-time concerns) to handle linking and dependency synchronization.