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

fix(vite): vitest migration add reporters #20823

Merged
merged 1 commit into from
Dec 18, 2023

Conversation

mandarini
Copy link
Member

  • Fixed vitest
  • Fixed mode for serve & preview

Related Issue(s)

Fixes #20816 #20804

@mandarini mandarini requested a review from a team as a code owner December 18, 2023 15:05
Copy link

vercel bot commented Dec 18, 2023

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

1 Ignored Deployment
Name Status Preview Updated (UTC)
nx-dev ⬜️ Ignored (Inspect) Visit Preview Dec 18, 2023 3:21pm

@mandarini mandarini requested a review from jaysoo December 18, 2023 15:08
@mandarini mandarini force-pushed the fix/vitest-migration branch from 6fdbf4e to fa85165 Compare December 18, 2023 15:20
@mandarini mandarini enabled auto-merge (squash) December 18, 2023 15:21
Comment on lines +41 to +42
...options,
...buildTargetOptions,
Copy link
Contributor

Choose a reason for hiding this comment

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

So buildTarget options should override the serve target options? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, because ideally there would not be any overriding done, since there are no common options in serve and build. In any case, it's better to configure your project in vite.config.ts!

if this surfaces any errors we can revisit, but we're trying to keep the executor logic as minimal as possible.

Copy link
Member Author

Choose a reason for hiding this comment

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

I see how this may seem confusing or wrong, however here's the thing:

When the options are loaded in the executor, the executor loads the main options from build, along with the options under the development configuration, for example:

    "build": {
      "executor": "@nx/vite:build",
      "outputs": ["{options.outputPath}"],
      "defaultConfiguration": "production",
      "options": {
        "outputPath": "dist/apps/my-app",
        "generatePackageJson": false
      },
      "configurations": {
        "development": {
          "mode": "development",
          "watch": true
        },
        "production": {
          "mode": "production"
        }
      }
    },
    "serve": {
      "executor": "@nx/vite:dev-server",
      "options": {
        "buildTarget": "my-app:build"
      },
      "configurations": {
        "development": {
          "buildTarget": "my-app:build:development",
          "hmr": true
        },
        "production": {
          "buildTarget": "my-app:build:production",
          "hmr": false
        }
      }
    },

It's up to the user to make sure these are set the way the user prefers.

In this case, it would load from build the following:

{
        "outputPath": "dist/apps/my-app",
        "generatePackageJson": false,
        "mode": "development",
         "watch": true
 }

And the options that would be loaded from serve would be:

{
          "buildTarget": "my-app:build:development",
          "hmr": true
 }

All these will be simplified in the near future, when we will be moving away from executors, and the logic will be much more transparent.

Copy link
Contributor

@viniciusbsneto viniciusbsneto Dec 18, 2023

Choose a reason for hiding this comment

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

Oh! I see! serve and build targets have different schemas.

So that's why ideally there shouldn't be any overrides. It's the extra args that kinda allows the overriding. That got me confused. Although it's possible to setup mode in the serve target options, it shouldn't be done. Is that correct?

Thanks for the taking the time to explain! 🙇🏻‍♂️

@mandarini mandarini merged commit de0d238 into nrwl:master Dec 18, 2023
6 checks passed
@jindong-zhannng
Copy link

Hi friends. I am facing the reporter problem in my project now. Migrating to v17.2.7 seems not working for me, so do I have to wait for a v17.3.x version since I see this PR's migration has a 17.3.0-beta.0 tag?

Copy link

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 Dec 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
4 participants