embroider-build / embroider

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

Allow concurrent builds #2086

Open simonihmig opened 2 months ago

simonihmig commented 2 months ago

Opening this to explore if we can support running production builds (for the same app) concurrently. Use case is to build the same app, but with different config, e.g. for different environments like changing CDN host names.

Obviously, the webpack/vite output directory would need to be different. But that should be easy to do, like using an environment variable that gets read from in the webpack/vite config.

The part that seems to require changes here would be avoiding conflicts for the stuff that Embroider writes into node_modules/.embroider.

Any concerns for trying to make this folder more dynamic, like making this have a random part like node_modules/.embroider-<random-hash-like-string>? And having that by default, would that cause performance regressions for example, like does Embroider reuse (parts of) that output between builds (which wouldn't work when it was randomized)?

On the implementation side, seem there is already a central place where this folder location is determined, would it be as simple as adding the randomization there, or are there more things to be aware of?

mansona commented 2 months ago

This feels very much like the vite Environment API that is upcoming in v6 https://github.com/vitejs/vite/pull/16471 🤔 do you think that covers your usecase?

simonihmig commented 2 months ago

do you think that covers your usecase?

Idk, have no deep understanding of that new feature, but it seems to me this is not able to address the fact that Embroider(!) is writing into a single node_modules/.embroider folder?

simonihmig commented 2 months ago

For the record, we discussed this in the team meeting, and there was consent that we could get this supported (as in "PR is welcome" 😄 )

Worth noting that Embroider does reuse artefacts in node_modules/.embroider between builds, so a randomly created folder name would work well here. We could instead expose an environment variable as a low level primitive to tell Embroider which directory name to use. So it would be up to the consumer to decide where we need a different one, like when we have two configuration sets, say 'us' and 'eu', we could pass EMBROIDER_BUILD_DIRECTORY=.embroider-us (matching DEPLOY_TARGET), so Embroider would write into node_modules/.embroider-us or node_modules/.embroider-eu, depending on that var.

amk221 commented 1 month ago

I think we're experiencing a problem here.

We run

"watch": "ember server --environment=development --port=4200",
"watch:tests": "ember server --environment=test --port=4201 --output-path=tmp/watch-tests-dist"

Two scripts

  1. a normal ember server, which has a Webpack plugin, that on done will does some jiggery pokery and then copies the output files to a different location

  2. a normal test server, but outputing the dist files in a different location, so as to not trample on the normal watcher script above.

The problem we are now experiencing having moved to Embroider is, if we have the watch script running, at the same time as watch:tests, then it will inherit the environment of test, which then breaks development builds.

mansona commented 1 month ago

@amk221 is there any particular reason why you can't just run ember server and go to tests at localhost:4200/tests/ ? This should always work, we even make sure that it works in the vite build 🤔

mansona commented 1 month ago

Idk, have no deep understanding of that new feature, but it seems to me this is not able to address the fact that Embroider(!) is writing into a single node_modules/.embroider folder?

@simonihmig the thing about Main with vite now is that only rewritten packages are getting written to the .embroider folder now, so the example that you're talking about: changing CDN host names is actually an app concern and doesn't hit the .embroider folder at all.

Obviously you're stuck on main being released as stable but this same idea will be true for Webpack inversion of control too

amk221 commented 1 month ago

@mansona

Because if you run ember serve, then the environment will be development, Meaning publicAssetURL will be computed for development, which will end up being something like https://some.cdn.dev. So if we visit http://localhost:4201/tests it will try to load the JS (and tests) from the cdn, but it should not. That's why we explicitly run tests with the test environment, not development.

mansona commented 1 month ago

so that's not actually true 🙈 and if you are experiencing that somewhere then you have a bug.

Running ember server and going to the app you should get the development version of your environment/config and going to your localhost:4200/tests it should be loaded with the test version

This is true for @embroider/macros too i.e. isTesting() is true only when you visit localhost:4200/tests

amk221 commented 1 month ago

I'm 99% sure its true, as in, the test HTML file is computed with URLs pointing to a publicAssetURL for the development environment.

Example: https://github.com/amk221/-embroider-concurrent-builds/commit/6ac33fb0721c12c33e33b4a0f37ab3675b0f82d2

simonihmig commented 1 month ago

My understanding was also that since basically forever if you go to localhost/tests, you are still serving the same build, which was created using development. Like in ember-cli app.env or process.env.EMBER_ENV is development!

changing CDN host names is actually an app concern and doesn't hit the .embroider folder at all.

My concern is not having that config available in rewritten packages. That example is an app level concern indeed. But we cannot build the same app in parallel, with different configs, when all those builds write into the same .embroider folder.