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

Allow concurrent builds #2086

Open
simonihmig opened this issue Aug 30, 2024 · 10 comments
Open

Allow concurrent builds #2086

simonihmig opened this issue Aug 30, 2024 · 10 comments
Labels
enhancement New feature or request

Comments

@simonihmig
Copy link
Collaborator

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?

@simonihmig simonihmig added the enhancement New feature or request label Aug 30, 2024
@mansona
Copy link
Member

mansona commented Sep 3, 2024

This feels very much like the vite Environment API that is upcoming in v6 vitejs/vite#16471 🤔 do you think that covers your usecase?

@simonihmig
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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
Copy link

amk221 commented Oct 7, 2024

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
Copy link
Member

mansona commented Oct 7, 2024

@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
Copy link
Member

mansona commented Oct 7, 2024

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
Copy link

amk221 commented Oct 7, 2024

@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
Copy link
Member

mansona commented Oct 7, 2024

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
Copy link

amk221 commented Oct 7, 2024

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
Copy link
Collaborator Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants