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

Command Watcher #1877

Merged
merged 7 commits into from
Apr 15, 2024
Merged

Command Watcher #1877

merged 7 commits into from
Apr 15, 2024

Conversation

BlueCutOfficial
Copy link
Collaborator

@BlueCutOfficial BlueCutOfficial commented Apr 11, 2024

Context

When working with vite, running the dev server and generating the production build with Rollup are two different actions that both have a specific execution. For instance, when dealing with public assets provided by the app and addons:

  • when running the vite dev server, the assets plugin uses a middleware to catch the assets-related request, find the actual location of the asset, and serve it.
  • when building the app (like for a production build), the assets plugin will emit the public assets in the dist/ output, where they are supposed to be.

This case highlights why we need to test some behaviors for both dev mode and build mode now the test suite is using vite.

Testing dev mode easily

This PR implements a CommandWatcher utility to ease running a vite dev server in tests. It relies on the former EmberCLI class that was used to run ember serve, but it makes it more generic (you can now choose the name of the command like ember or vite) and it makes it an importable helper so we can test the dev mode in multiple test files.

Compat dummy app test

The first use case for the new CommandWatcher is the compat-dummy-app-test. This PR prepares the field so the tests that check public assets presence now use vite and check both the dev mode and the build.

This preparation work is required to complete #1859, which will remove the part of the code that is now managed by the assets plugin: in #1859, compat-dummy-app-test will have to stop testing public assets are present in the rewritten app and instead test they are present in the dist (in build mode) or served correctly (in dev mode).

Here, aside of the assertions, we do the following things:

  • We update the addon template used for the dummy app test scenarios. vite is now configured to run the assets plugin and we also update ember-qunit to fix an issue in the development build.
  • The test waits for the dev server to be ready, and it knows when it's ready using a regexp matching the text that is supposed to be output on the server when it's started. This regexp is now slightly more flexible, and in CommandWatcher implementation, we strip the Ansi characters before analyzing the server output, because vite server colorize the text, which can mess-up the regex.

@@ -81,6 +82,7 @@
"ember-source-latest": "npm:ember-source@latest",
"ember-truth-helpers": "^3.0.0",
"execa": "^5.1.1",
"node-fetch": "2.7.0",
Copy link
Collaborator Author

@BlueCutOfficial BlueCutOfficial Apr 11, 2024

Choose a reason for hiding this comment

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

I had to use [email protected] because > 3.x is an ESM-only package. Importing this into the scenarios files triggers an Unknown file extension ".ts" error.

@@ -81,6 +82,8 @@
"ember-source-latest": "npm:ember-source@latest",
"ember-truth-helpers": "^3.0.0",
"execa": "^5.1.1",
"node-fetch": "2.7.0",
"strip-ansi": "^6.0.0",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I had to use [email protected] because > 7.x is an ESM-only package. Importing this into the scenarios files triggers an Unknown file extension ".ts" error or an ERR_REQUIRE_ESM error.

@BlueCutOfficial BlueCutOfficial marked this pull request as ready for review April 12, 2024 13:26
ef4 and others added 3 commits April 12, 2024 18:05
… flexible regexp to wait for the server and call strip-ansi because vite output contains colors messing up with theregexp
…don to make sure the publicDir config doesn't interfeer with addons
Comment on lines +24 to +26
public: {
'from-addon.txt': 'a public asset provided by the classic addon',
},
Copy link
Collaborator Author

@BlueCutOfficial BlueCutOfficial Apr 12, 2024

Choose a reason for hiding this comment

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

Maybe overkill but I added a public asset at the root to show that changing the publicDir location (that allows the public assets of the dummy app to be copied in the dist) does not impact how the addon's public assets are served and added to the dist by the assets plugin.

assets.includes('./addon-template/from-addon.txt');
});

test('production build contains public assets from both addon and dummy app after a build', async function (assert) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This test builds the dummy app the same way as the one checking the rewritten-app so I could have done all the checks in the same test. I preferred separated tests because I want to remove the rewritten-app one in PR #1859

Copy link
Member

@mansona mansona left a comment

Choose a reason for hiding this comment

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

Amazing work 🎉

@mansona mansona added enhancement New feature or request internal and removed enhancement New feature or request labels Apr 15, 2024
@mansona mansona merged commit 445e96b into main Apr 15, 2024
90 of 91 checks passed
@mansona mansona deleted the command-watcher branch April 15, 2024 08:56
@github-actions github-actions bot mentioned this pull request Apr 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants