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

chore!: update vite@5 and rollup@4 #4368

Merged
merged 34 commits into from
Nov 10, 2023
Merged

Conversation

Dunqing
Copy link
Member

@Dunqing Dunqing commented Oct 25, 2023

Description

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

  • It's really useful if your PR references an issue where it is discussed ahead of time. If the feature is substantial or introduces breaking changes without a discussion, PR might be closed.
  • Ideally, include a test that fails without this PR but passes with it.
  • Please, don't make changes to pnpm-lock.yaml unless you introduce a new test example.

Tests

  • Run the tests with pnpm test:ci.

Documentation

  • If you introduce new functionality, document it. You can run documentation with pnpm run docs command.

Changesets

  • Changes in changelog are generated from PR name. Please, make sure that it explains your changes in an understandable manner. Please, prefix changeset messages with feat:, fix:, perf:, docs:, or chore:.

@stackblitz
Copy link

stackblitz bot commented Oct 25, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@netlify
Copy link

netlify bot commented Oct 25, 2023

Deploy Preview for fastidious-cascaron-4ded94 canceled.

Name Link
🔨 Latest commit 56284cf
🔍 Latest deploy log https://app.netlify.com/sites/fastidious-cascaron-4ded94/deploys/654cf3edc745dc0008a0e94e

packages/vitest/LICENSE.md Outdated Show resolved Hide resolved
@sheremet-va
Copy link
Member

I want to merge this, but PR has merge conflicts. @Dunqing can you resolve them?

@Dunqing
Copy link
Member Author

Dunqing commented Oct 30, 2023

I want to merge this, but PR has merge conflicts. @Dunqing can you resolve them?

Sure!

packages/vitest/rollup.config.js Outdated Show resolved Hide resolved
packages/vitest/rollup.config.js Outdated Show resolved Hide resolved
packages/vitest/rollup.config.js Outdated Show resolved Hide resolved
packages/vitest/package.json Outdated Show resolved Hide resolved
@Dunqing Dunqing force-pushed the chore/rollup4 branch 2 times, most recently from e7cdf7a to 3523a6a Compare November 1, 2023 07:58
@Dunqing
Copy link
Member Author

Dunqing commented Nov 1, 2023

I roughly compared the ci test time between this branch and the main branch, and the testing time seems to have decreased 😀

@sheremet-va
Copy link
Member

We should wait for vitejs/vite#14833 to be released, so we can import parseAstAsync from vite directly

@Dunqing
Copy link
Member Author

Dunqing commented Nov 1, 2023

We should wait for vitejs/vite#14833 to be released, so we can import parseAstAsync from vite directly

Should we specify version 5 for vite, otherwise we don't have parseAst

@sheremet-va
Copy link
Member

We should wait for vitejs/vite#14833 to be released, so we can import parseAstAsync from vite directly

Should we specify version 5 for vite, otherwise we don't have parseAst

Yep

@gajus
Copy link

gajus commented Nov 1, 2023

FYI Vite released the changes.

Thank you for this PR @Dunqing

@Dunqing Dunqing changed the title chore: upgrade to rollup 4 chore!: update vite@5 and rollup@4 Nov 2, 2023
@sheremet-va
Copy link
Member

Interesting errors in your tests 🤔

@Dunqing
Copy link
Member Author

Dunqing commented Nov 2, 2023

Interesting errors in your tests 🤔

Looking good now.

Recent tests seem to have some unusual errors, and not just in this branch.

@gajus
Copy link

gajus commented Nov 2, 2023

@sheremet-va Is there anything else that needs to be done here?

@ghiscoding
Copy link
Contributor

Vite is now at 5.0.0-beta.16, does this PR need another update? It looks like Vite is getting closer to a release since a PR for Vite 5 announcement blog post was created. I guess that Vitest is not quite ready for the official 1.0 yet, right? From the Vitest 1.0 roadmap, it looks like migrating vite-node into Vite is the biggest task left but I assume this might not happen right away since Vite 5 is very close to be shipped?

Thanks for the great work 🎉

@sheremet-va
Copy link
Member

vite-node will not be in Vite 5, yes.

not sure what update this PR needs tho, can you elaborate?

@ghiscoding
Copy link
Contributor

ghiscoding commented Nov 4, 2023

vite-node will not be in Vite 5, yes.

not sure what update this PR needs tho, can you elaborate?

just an update from Vite Beta.15 to Beta.17, nothing else

@sheremet-va sheremet-va merged commit a2d7565 into vitest-dev:main Nov 10, 2023
14 of 16 checks passed
mbland added a commit to mbland/vitest-utils-browser-import that referenced this pull request Dec 6, 2023
This took a bit of figuring out. The process was:

- Add `.npmrc` to this repo per:
  - https://pnpm.io/npmrc#auto-install-peers
  - https://pnpm.io/npmrc#prefer-frozen-lockfile
  - https://pnpm.io/npmrc#prefer-workspace-packages

- `git clone https://github.com/vitest-dev/vitest.git` in the parent
  directory of this repository.

- Add 'file:' dependencies to vitest and @vitest/browser, both residing
  within '../vitest/packages'.
  https://pnpm.io/cli/link#whats-the-difference-between-pnpm-link-and-using-the-file-protocol

- Add pnpm-workspace.yaml with an '../vitest/packages/*' entry so the
  local vitest modules can find their peer modules in the same
  workspace.
  https://pnpm.io/pnpm-workspace_yaml

- Set vite to 4.5.1, since I eventually learned the problem always
  occurs with vite 5.0.5. (I suspect this may have something to do with
  the upgrade from Rollup 4 to Rollup 5.)

Then in the `../vitest` directory:

```sh
git reset --hard v1.0.0-beta.4
pnpm i --ignore-scripts --frozen-lockfile
pnpm build
```

And back in this repository:

```sh
pnpm i
pnpm test
```

I can now get the demo test running successfully against the local
vitest v1.0.0-beta.4. Repeating the process for v1.0.0-beta.5, I can
reproduce the vitest/utils import problem/reload loop.

Now let's see what a `git bisect` will produce...

...one thing I already noticed:

- The vitest packages from v1.0.0-beta.4 depend on vite < 5.0.0
- The vitest packages from v1.0.0-beta.5 depend on vite >= 5.0.0-beta*

So...could be the issue resides within Vite somehow.

The vitest repo is OK up through this commit, which is the last working
commit before the Vite ^5.0.0 bump:

- commit f6a445dbc86e0e5b7de29ae650ecebf8ac6e4ffa

The next commit breaks the pnpm-lock.yaml file, and remains broken until
the following commit and the PR containing it gets merged:

- commit 5b738663197b78cf103b269c6e7489286957c1a7
- vitest-dev/vitest#4368

```sh
git diff f6a445dbc86e0e5b7de29ae650ecebf8ac6e4ffa 5b738663197b78cf103b269c6e7489286957c1a7
```

Yep. None of the changes there seem to have anything directly to do with
packages/browser/src/client/main.ts or the `config.base` value. It
seems to be the Vite 5 upgrade, which is consistent with the fact that
v1.0.0-beta.4 works with Vite 4.5.1, but not Vite 5.0.5.

Now that I've got my methodology down, next I'll start bisecting commits
in Vite.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants