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

feat(angular): add vitest option to angular #27311

Merged
merged 34 commits into from
Oct 29, 2024

Conversation

yjaaidi
Copy link
Contributor

@yjaaidi yjaaidi commented Aug 6, 2024

  • This adds vitest option to unitTestRunner for Angular generators.
  • This does not add vitest option to create-nx-workspace but I think we should provide this as an option in the future. Please let me know if you wantme to add this here or as a future feature.

@yjaaidi yjaaidi requested review from a team as code owners August 6, 2024 16:38
@yjaaidi yjaaidi requested review from Coly010 and jaysoo August 6, 2024 16:38
Copy link

vercel bot commented Aug 6, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Skipped Deployment
Name Status Preview Updated (UTC)
nx-dev ⬜️ Ignored (Inspect) Visit Preview Oct 29, 2024 3:36pm

Copy link

nx-cloud bot commented Aug 6, 2024

Copy link
Contributor

@Coly010 Coly010 left a comment

Choose a reason for hiding this comment

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

Looks great! I need to test it out properly, but other than the comments I've added, I don't see anything untoward.

Could you add some e2e tests that cover using Vitest as a selected option so that we can ensure behaviour continues to work going forward?

packages/angular/src/generators/utils/add-vitest.ts Outdated Show resolved Hide resolved
packages/angular/src/generators/utils/add-vitest.ts Outdated Show resolved Hide resolved
packages/angular/src/utils/versions.ts Outdated Show resolved Hide resolved
packages/vite/src/generators/configuration/schema.d.ts Outdated Show resolved Hide resolved
@yjaaidi
Copy link
Contributor Author

yjaaidi commented Aug 7, 2024

Looks great! I need to test it out properly, but other than the comments I've added, I don't see anything untoward.

Could you add some e2e tests that cover using Vitest as a selected option so that we can ensure behaviour continues to work going forward?

Of course! By the way, I also wanted to add an e2e to test the watch mode which is generally a common issue. As a matter of fact, the watch was detecting file changes and running the tests without rebuilding the TS. For some reason, I had to upgrade the angular plugin to 1.7.0 to make it work.
🤔 I wonder if it is worth exploring e2e tests for watch mode which can be pretty tricky.

@Coly010 Coly010 force-pushed the feat/add-vitest-option-to-angular branch 2 times, most recently from d5136c4 to 03bdf3d Compare September 11, 2024 10:08
* @param path The path relative to the workspace root to start searching from.
* @param fileName The name of the file to search for.
*/
export function findFileInClosestParentFolder(
Copy link
Member

Choose a reason for hiding this comment

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

It looks like this function is used for the purpose of detecting whether Node will treat a .js or .ts file as ESM or not (due to "type": "module" in package.json).

My suggestion is to explicitly use .mts (or .mjs, .cjs, .cts) file extensions so the format is controlled rather than let Node interpret it. In that case, we don't need this function in devkit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly! I thought that using .ts when possible would be less confusing to users and machines.
For instance, Playwright VSCode Plugin does not detect playwright.config.mts files. So there might be some plugins/tools that do not detect vite.config.mts.

Other than that, if you are fine with generating vite.config.mts all the time, we can get rid of this function and I'll be happy to fix this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's generate vite.config.mts all the time and we can remove this function :)

@the-ult
Copy link

the-ult commented Oct 21, 2024

Is there any news on an ETA when this awesome new feature is gonna be available?

@yjaaidi yjaaidi force-pushed the feat/add-vitest-option-to-angular branch from 70db017 to 8262f3c Compare October 29, 2024 00:00
Copy link

vercel bot commented Oct 29, 2024

Deployment failed with the following error:

The provided GitHub repository does not contain the requested branch or commit reference. Please ensure the repository is not empty.

@yjaaidi
Copy link
Contributor Author

yjaaidi commented Oct 29, 2024

Is there any news on an ETA when this awesome new feature is gonna be available?

Hey @the-ult , thanks for your interest to this PR.
I just made the changes requested by @jaysoo so hopefully it could ship soon.

@the-ult
Copy link

the-ult commented Oct 29, 2024

Is there any news on an ETA when this awesome new feature is gonna be available?

Hey @the-ult , thanks for your interest to this PR. I just made the changes requested by @jaysoo so hopefully it could ship soon.

Awesome.
Thanks for all the great work!

@Coly010 Coly010 force-pushed the feat/add-vitest-option-to-angular branch from c3db560 to 3571d5a Compare October 29, 2024 11:03
Copy link
Contributor

@brandonroberts brandonroberts left a comment

Choose a reason for hiding this comment

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

🫡

@yjaaidi
Copy link
Contributor Author

yjaaidi commented Oct 29, 2024

@analogjs/vitest-angular is moving fast. There's a tradeoff there, but shouldn't we use a caret match instead of holding users back?

@Coly010
Copy link
Contributor

Coly010 commented Oct 29, 2024

@analogjs/vitest-angular is moving fast. There's a tradeoff there, but shouldn't we use a caret match instead of holding users back?

I'd much rather pay the maintenance tax for updating minors more frequently given we support current Angular Major + 2 previous major versions with Nx. We can ensure better stability with all the versions we support this way.

@jaysoo jaysoo merged commit 9fe8274 into nrwl:master Oct 29, 2024
4 of 6 checks passed
@wall-street-dev
Copy link

Oh s*** back to Nx again.... cool stuff @yjaaidi 🔥🔥🔥

@ErickRodrCodes
Copy link

This is going to be so 🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥, Now we have to wait for serving/building to be under vite.

Copy link

github-actions bot commented Nov 5, 2024

This pull request has already been merged/closed. If you experience issues related to these changes, please open a new issue referencing this pull request.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants