Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable Embroider #746

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Enable Embroider #746

wants to merge 1 commit into from

Conversation

thoov
Copy link
Member

@thoov thoov commented May 7, 2021

Enable Embroider RFC

Co-authored-by: Katie Gengler <katie@kmg.io>
Co-authored-by: David Baker <acorncom@users.noreply.github.com>

While Embroider strives to be as backwards compatible as possible, some existing concepts will not directly apply under Embroider. These concepts largely rely around the fact that Embroider does not want to you care about the contents of your output. This means that concepts like fingerprints asset file names or modifing output paths of your dist assets will not be supported.

#### Fingerprinting
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bringing over a comment thread from a predraft version of this RFC:

@kategengler:
Are other fingerprint options still supported?

@thoov:
Currently no. I can make this section state more explicitly that app.options.fingerprint are not enabled

@simonihmig:
I think we could (should?) point out that there is still quite a gap in the functionality, as Embroider will only fingerprint the basic JS and CSS files that it adds to index.html. broccoli-asset-rev would fingerprint arbitrary files, e.g. images, and rewrite all the references to those (as long as it can "see" the file paths, so they have to be literals, i.e. no dynamic concatenations logic or so), e.g in JS or CSS files. AFAIK there is no solution for this yet in Embroider-land.

Which is quite annoying as you cannot serve images with HTTP headers that enable long-term browser caching as you normally would. Or if you do, they are not invalidated when they change.

And people are probably quite used to this (the previous behavior), so when this does not work it would feel like a bug rather than a missing feature.


### Community Adoption

Embroider relies on all addons being able to be compliant with the V2 package spec. Embroider is itself able to handle most addons by converting them to V2 but some addons are too dynamic to be automatically converted. As the community adopts Embroider, more and more community addons will need to be fully working under Embroider to ensure its successful adoption. Embroider has created a [testing package](https://github.com/embroider-build/embroider/blob/master/ADDON-AUTHOR-GUIDE.md) for addon authors to consume in their ember-try scenarios to verify their addons work under Embroider. A meta issue will be created tracking the top 100 addons according to emberobserver.com. This issue will track the adoption of @embroider/test-setup for each addon and help create a working list of which addons work under Embroider and which ones need help.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there plans to incorporate an addon's Embroider-compatibility on its Ember Observer page or listing by default?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, with the caution that Ember Observer isn't an Ember-org project. I plan to indicate both 'Embroider-safe' and whether the addon is v2. Ideally I would already have done this but unfortunately it is a side-project...


The migrate command is purely to codemod the file changes above to get your app onto Embroider.

### Deprecations

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe some of the existing Ember CLI build hooks used by addons are no longer supported in Embroider land. Can we be more explicit about what these hooks are, and if possible some guidance on how to migrate away from using them?

Should we also have official deprecations for these unsupported hooks?

@simonihmig
Copy link
Contributor

Adopting this RFC - which I am totally in favor - determines if and how we do the Embroider switch, but the when is also important. As I argued earlier, there are a couple of issues with very popular addons like ember-cli-sass or ember-intl. So we must make sure that the switch only happens when the overall stability is at least "good enough", to not make the whole "ember experience" feel broken.

It's one thing to let devs opt in to something with a more or less beta-like stability, and another to make it the default even for new users.

Idk if it's ok for the RFC to just focus on the if and how, or if we should get more explicit about the when here? Sure enough it's hard to objectively define when things are "good enough". Maybe at least commit to a tracking issue where anything that reaches the bar of a "blocker" (again, hard to define) is listed?

@timiyay
Copy link

timiyay commented May 11, 2021

Is the intent to introduce Embroider-by-default in Ember 4.0?

Our experience of many aborted attempts to start using Embroider is that the Ember addon ecosystem has some non-trivial incompatibilities, as @simonihmig mentions above.

Any packages related to SASS/CSS buildchains, in particular, have been showstoppers for us (ember-component-css and ember-css-modules are the main ones). ember-cli-mirage is also rife with dynamically-generated code that we need to work though.

All in all, is it viable to enable Embroider within v3 and expect it to be "not broken"?

@NullVoxPopuli
Copy link
Sponsor Contributor

NullVoxPopuli commented May 12, 2021

I have embroider on a few v3 projects and it works fine. Even with postcss+tailwind!

I'm not using any of those add-ons though.

@cah-brian-gantzler
Copy link

cah-brian-gantzler commented Jul 16, 2021

ember-cli-mirage is also rife with dynamically-generated code that we need to work though.

@timiyay Could you detail what you think some of that dynamically generated code is? Doing some work on ember-cli-mirage and would like to address those if possible.

@timiyay
Copy link

timiyay commented Jul 17, 2021

@cah-briangantzler I'll circle back to our embroider PRs, revive it, and dig up the stack traces.

It's possible our long-term usage of Mirage gives us a misunderstanding of how it works today, relating to how Mirage models and serializers work with ember-data.

I'll open issue(s) on ember-cli-mirage if I can repro. Update: here's the first issue we hit miragejs/ember-cli-mirage#2212

@ef4
Copy link
Contributor

ef4 commented Oct 27, 2021

A followup toward addressing concerns raised above:

  1. For fingerprinting, the goal will be to iterate and land Asset Import Spec #763 with a solution that works in both classic and embroider.

  2. For addons that extend the build pipeline like ember-cli-sass, ember-css-modules, etc: the goal here is to stop needing to make ember-specific wrappers around that kind of functionality. When building with webpack, there are perfectly good webpack solutions for each of those things. The same will be true for whatever alternative bundlers people may adopt in the future.

  3. ember-intl falls partly into that same category. Partly it provides runtime API like helpers and services, which is straightforward to support under Embroider. But partly it extends the build pipeline, so the reliable way for it to do that under Embroider is to tell app authors how to plug it into their build, with directions specific to the bundlers that it actually supports. For example, it could tell people to do this for webpack:

    compatBuild(app, Webpack, {
      packagerOptions: {
        webpackConfig: {
          rules: [
    +         require('ember-intl/webpack-rule')
          ]
        }
    })

    This is more explicit than people are used to with addons, but that is why addons have been locked into a single inflexible build system for so long.

@SergeAstapov
Copy link
Contributor

@ef4 for the statement 3: does it mean each addon will need to explicitly support specific bundler, e.g. given example above it would be responsibility of ember-intl to provide bindings for webpack, vite, etc.?

@ef4
Copy link
Contributor

ef4 commented Oct 27, 2021

Yes, but only addons that need to extend the build system. Most addons don't need to do that, and many addons that have custom broccoli code today only do so to achieve things that they can achieve with @embroider/macros instead, which are portable to any build system.

@jrjohnson
Copy link
Sponsor

jrjohnson commented Oct 29, 2021

would be responsibility of ember-intl to provide bindings for webpack, vite, etc

Wait wut? I understand that Embroider unlocks the option to try out different build tooling, but I expected there would be a one true path™️ that the community would support. Now I'm terrified that some of the most critical addons in the ember ecosystem are going to assume a huge maintenance and testing burden to provide different bundler-of-the day options. I agree this isn't a problem for most addons, but it is a really really big problem for a few and those few are used by huge swaths of the community.

that is why addons have been locked into a single inflexible build system for so long

I'd re-phrase this as "been able to provide an awesomely easy install experience with no need for most app consumers to understand anything about the underlying build system at all"

That's been a huge plus for ember. No build config. I'd assumed through the embroider RFC process that we'd keep that, but are we going to have 6 ember-intl addon forks in order to keep up with fads in JS tooling?

@ef4
Copy link
Contributor

ef4 commented Oct 29, 2021

I very much expect most people to use the default, which will be webpack for the foreseeable future. So no, I don't expect any explosion of burden for the small number of addon authors who really need to extend the build pipeline. The point here is to be transparent about what is supported, so that the set of steps required to port an app from one tool to another is more obvious.

I can also see how this sounds worse than it is if you haven't had a chance to work with the various plugin architectures that are out there. The kind of transformations that it's legitimate for an addon to want to do are much easier to do as a webpack loader or rollup plugin, etc, than as broccoli transforms. You will find far more people capable of contributing to them and far more learning materials on how to write them.

The amount of code that's specific to one bundler is typically a dozen lines. For example, this is what a realistic webpack loader looks like and this is what a realistic rollup plugin looks like. They should be short and sweet, and all the meat of your addon should be shared code that's agnostic to the bundler.

but are we going to have 6 ember-intl addon forks in order to keep up with fads in JS tooling?

No forking is required. If our APIs are well-designed, they lean into Javascript semantics and whatever extensions they do on top of that are extremely thin. We get future-safety through web standards, not through locking in implementations. An example of what I mean by this is: the safe way to implement ember-intl is to have it translate what-is-authored to a thing expressed in terms of ES modules connected by imports. As long as you do that translation into standards-compliant javascript, you won't need to do anything special for any standards compliant bundler to finish the job.

IMO, you're conflating the faddishness of JS libraries (which is real) with build tooling. Build tooling is much less subject to fads, because it's much more capital-intensive. There are really not very many credible contenders. Are there even six different JS bundlers that saw widespread adoption in, say, the past five years? I can't come up with that many. I think realistically there are two dominant plugin architectures (Webpack and Rollup) and then things that are plugin-compatible with them (Vite and Snowpack use Rollup), precisely because nobody wants to rewrite all their plugins.

I'm open in this RFC to discussing ways to both document clear default expectations for addon authors, so that there's one best practice thing to do at any given point in time. And I'm also open to hearing suggestions about how to provide more automatic glue without making extensibility impossible.

My opinion after long years of studying how software ecosystems evolve is that the ideal amount of code you need to install a plugin is not zero lines. It's two lines (import it so you see where it comes from, and invoke it in the right place in your config). The difference may seem small, but it greatly eases learning, understanding, and empowering devs. People need to see how their choices come together. Having a single place that governs your build pipeline and seeing each of the dependencies that you chose interacting there is indeed more code to look at. But when you don't have it, the learnability of the system is far worse. Very few people have gotten over the hurdles of learning broccoli well enough to actually contribute to code using it. It actively hurts our ecosystem that we hide so much away and make it so hard to learn.

@chriskrycho
Copy link
Contributor

I’d just like to very strongly second @ef4 here:

…the ideal amount of code you need to install a plugin is not zero lines. It's two lines (import it so you see where it comes from, and invoke it in the right place in your config). The difference may seem small, but it greatly eases learning, understanding, and empowering devs. People need to see how their choices come together. Having a single place that governs your build pipeline and seeing each of the dependencies that you chose interacting there is indeed more code to look at. But when you don't have it, the learnability of the system is far worse.

One of the most common complaints I hear, working as an infrastructure engineer at LinkedIn and supporting hundreds of engineers working on our app is that they can’t even figure out how to figure out how things fit together. We hire good engineers, many of whom come in wanting to learn and contribute, and a lot of them eventually give up.1 The idea of “zero config” sounds nice, but “zero config” is much better approached as “the average app requires zero manual config to get up and running, but all the config is available when you need to adjust it”—and, equally, that all the config can be followed.

I would analogize this to the move we’ve made with Ember’s test infrastructure (with both QUnit and Mocha) over the last few years. The design of Ember’s test tooling in the 1.x era and most of the 2.x era had many fewer imports—but it was actually harder to work with in the long term as a result than our current system, which adds just enough explicitness to allow you to follow the chain when you need to. For the cost of a couple extra lines at the top of each test module, we gained the ability for people to understand all of the “magic” in the system. Getting rid of moduleFor* in favor of setupTest and setupRenderingTest was massively clarifying. Getting rid of special knowledge about kinds of registered items (components, routes, services, etc.) in favor of treating them all the same and allowing you to choose to layer in additional capabilities as appropriate (and providing good blueprints so you don’t have to understand that to get started) makes the system far easier to come up to speed on,t o change when you need your own capabilities, and (importantly) to change over time.

This doesn’t go forever. You can end up with too much explicitness for a given context. The goal is to be as (but only as) explicit as is appropriate for the domain we’re working in.2 We don’t need to be talking about memory allocation or handling CPU registers in our build config. But we do need the ability to defineall the pieces we’ve integrated into the build and how they relate to each other. And by the same token: it’s helpful as a ‘separation of concerns’ for a high-performance computing tool written in Rust to be able to reason about memory allocation… while having Cargo handle the vast majority of build work “transparently” (but configurably!).


One other note: I’ve spent much less time than Ed has with build tooling over the last few years. But I feel quite confident precisely because I lack his expertise that his claim about the ease of writing a Rollup or webpack integration is much, much higher than that of working with Broccoli plugins. I have bounced off of parts of Ember CLI and Broccoli repeatedly—enough to sometimes feel quite dumb as a result. By contrast, I’ve worked much more comfortably with both webpack and Rollup plugins, including in contexts where I was just building a small tool using one or the other from scratch. Add in the fact that in every case we will be speaking the lingua franca of spec-compliant JS, and I have no doubt that core ecosystem tooling will be easier to maintain, not harder, with this move.

Footnotes

  1. It’s easy for folks who have gotten over the hurdles of learning to be dismissive of the challenges: this is survivorship bias.

  2. I’m borrowing this framing and language from Dave Herman, Yehuda Katz, and Aaron Turon.

@jrjohnson
Copy link
Sponsor

I buy into that vision @ef4 / @chriskrycho. I'm not arguing with any of the points you've both made, they are excellent and good things. You're both talking about high level architecture and philosophy. Yay! Let's do this, it's great!

I'm talking about the practical concern that we're moving a lot of work to the edges without clear guidance on what that work is. I spend a significant amount of my OSS time updating addons and I can tell you that most of them are working very well in an almost completely unmaintained state. I fully support a move to standard build tooling and the less time I have to spend in AST explorer and debugger mode trying to worst through the build tree the better, but there is going to need to be a pretty big lift to get the top %% of addons on bored. I'm watching dedicated community members and maintainers try and get ember-get-config and ember-cli-page-object (for example) to work and it's not going super well. This is OK there will be pain in the transition and we can all pitch in and get this done, but if we have to do it all again when Webpack 6 is released it's going to be really draining.

I'm saying yes, let's go, but I'd like to see an update to this RFC or maybe just the communication around it that encourages a single path forward so we can all take the step together and then if something changes we'll all be ready to take the next step together. I just don't have a clear picture of exactly what only addons that need to extend the build system includes. It seems like a big chunk of addons I rely on need to do this now. Maybe they won't anymore, but I can't quite wrap my head around how to discover that.

@chriskrycho
Copy link
Contributor

chriskrycho commented Oct 29, 2021

The vast majority of addons shouldn't need any build time integration anymore. Many addons which “have a build” still won’t need build-time integration in the sense they have historically: they will just publish JS ready to be pulled in and used directly by consumers. (You also shouldn’t expect most addons to change their state of running on autopilot.)

Looking at what e.g. ember-cli-page-object is doing, I don’t think any of the things it does in the build pipeline currently need to happen at all, and in fact they only exist because of a bad tendency in an earlier era of Ember to rewrite imports. 😑 The solution there, having spent only the last couple minutes looking at it, is almost certainly just to cut a breaking change which just uses spec-compliant, Node-resolveable import paths, and then there will be no need for build integration at all. That’s going to be the case for many of these.

ember-get-config is a bit less standard in that it's trying to provide something to an addon from the host. I can think of a variety of ways of doing that (including the one it recommends: using the DI container) which don't involve build integration. Many of these kinds of things can also just be provided via changes to APIs—the assumption that this kind of thing should be available this way just may not be a great assumption (even if it's sometimes a convenient one).

In yet other cases which have historically required build integration—think here of CSS Modules—we may be able to actually just drop all of our special sauce in favor of just taking advantage of things like the webpack css-modules-loader.

As these suggest, part of the trick here is reevaluating whether a given approach actually makes sense in the Embroider world. Sometimes they will, and it'll just involve a small tweak. Sometimes, they won't, and that's fine, too.

Net, it's not clear to me why an exact breakdown for every single migration out there—they’re all different!—should be a blocker for moving forward with this RFC. Tackling it via docs, guidance, etc. as we go forward: absolutely!

@wagenet
Copy link
Member

wagenet commented Jul 23, 2022

I'm not quite sure if Embroider is ready to be the default yet, but it's definitely moving that way. Let me bring this up with the core team again and see what the thought is here.

@NullVoxPopuli
Copy link
Sponsor Contributor

NullVoxPopuli commented Jul 24, 2022

I think it's ready to be default. Why don't you think it is? 🙃

I do think that our --embroider flag should opt in to strictest settings by default.

@chriskrycho chriskrycho added the S-Exploring In the Exploring RFC Stage label Dec 1, 2022
@ef4
Copy link
Contributor

ef4 commented Jan 13, 2023

Update from framework core review meeting: We intend to do this but we expect this RFC will be superseded by one that actually defines the new app format, with some details that aren't in here yet. The existing compatBuild based embroider leaves a lot of crufty features in place that we don't really want to keep.

Examples:

  • the HTML format refers to outputs of the build rather than inputs of the build
  • the config/environment.js system is needlessly complicated
  • users should be able to see and use standard config files for the underlying tools like babel, webpack or vite.

We expect to land each of these things in embroider first and then summarize them in an RFC to propose making that the default.

@wagenet wagenet added the E-Polaris Work for the Polaris Edition label Mar 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E-Polaris Work for the Polaris Edition S-Exploring In the Exploring RFC Stage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet