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

Some changes to make vite-ecosystem-ci pass #371

Merged
merged 11 commits into from
Jul 23, 2022

Conversation

sapphi-red
Copy link
Contributor

This PR applies some change to make vite-ecosystem-ci pass.

bump pnpm version (719d1d3)

This is not related to vite-ecosystem-ci.

I think this packageManager field is outdated. The lockfile uses lockfileVersion 5.4 but this is supported from pnpm 7.0.0.
https://github.com/brillout/vite-plugin-ssr/blob/fe4ff6af483d03346eeef34904c6f3fca9dec161/pnpm-lock.yaml#L1

So I upgraded to 7.6.0.

set react-dom as peerDeps of @apollo/client (bc9591d)

examples/graphql-apollo-react was failing.
https://github.com/vitejs/vite-ecosystem-ci/runs/7462644762?check_suite_focus=true#step:7:478

This was because require('react-dom/server') inside @apollo/client was resolved to react 18.1.0 but react 18.2.0 was used by Vite.
I added react-dom to peerDep of @apollo/client by pnpm.packageExtensions. Maybe this should be fixed on package side? I'm not sure.

fix type error in examples/layouts-react/.testRun.ts (f7257e3)

examples/layouts-react had a type error.
https://github.com/vitejs/vite-ecosystem-ci/runs/7462644762?check_suite_focus=true#step:7:591

I simply fixed it.
BTW is there any tsconfig.json which covers files under examples directory? IIUC if there isn't any, the tsconfig in vite-ecosystem-ci will be applied.
Maybe the tsconfig in ecosystem-ci should exclude files in workspace directory so that it won't be applied to files which are cloned?

set ssr.noExternal to graphql-apollo-vue (e7238e1)

examples/graphql-apollo-vue was failing.
https://github.com/vitejs/vite-ecosystem-ci/runs/7462644762?check_suite_focus=true#step:7:751

When building client, @apollo/client, @vue/apollo-composable both resolves to module field.
When building SSR, these packages are externalized. But these packages does not have ESM entries, and these CJS entries are not compatible with node.js's automatic named export support.
So these packages needs to be bundled. I added them to ssr.noExternal.

Also I seems something strange is happening around package overrides.
When you run pnpm build in examples/graphql-apollo-vue, the output is:

vite v3.0.2 building for production...
// omit
vite v2.9.14 building SSR bundle for production...
// omit

So Vite 3 is used for client build and Vite 2 is used for SSR build. This maybe the reason why the CI is passing in this repository but not passing in ecosystem-ci.

(Not included in this PR) preact

Preact now supports Vite 3, so this code might be able to be removed after upgrading @preact/preset-vite to 2.3.0.
https://github.com/brillout/vite-plugin-ssr/blob/fe4ff6af483d03346eeef34904c6f3fca9dec161/boilerplates/.testRun.ts#L38-L44

@brillout brillout merged commit 86e4ddb into vikejs:main Jul 23, 2022
@brillout
Copy link
Member

I'm seeing that main is now failing, probably a minor thing I'll fix tomorrow. I'll reply to the rest tmrw as well. In the meantime: thanks for the PR!

@brillout
Copy link
Member

Alright, both the branch main (mostly Vite 2) and the branch vite3 (always Vite 3 with pnpm.overrides) are green.

I added react-dom to peerDep of @apollo/client by pnpm.packageExtensions. Maybe this should be fixed on package side? I'm not sure.

I think so too: github:apollographql/apollo-client - PR #9933 - file changes.

@apollo/client, @vue/apollo-composable [...] needs to be bundled. I added them to ssr.noExternal.

I can confirm that ssr.noExternal is needed. I tried to find a way to circumvent using ssr.noExternal but I failed and I don't think it's possible.

In care you are curious:

Hopefully, Apollo fixes these issues soon. I don't think that using ssr.noExternal is something the user should have to do.

Also I seems something strange is happening around package overrides. When you run pnpm build in examples/graphql-apollo-vue, the output is:

That's probably because vite-plugin-ssr automatically runs $ vite build --ssr on behalf of the user: https://github.com/brillout/vite-plugin-ssr/blob/55eb285f1b19baa39abe089687d0d9b90461701b/vite-plugin-ssr/node/plugin/plugins/autoFullBuild.ts#L39

(So that the user can simply run $ vite build instead of having to run $ vite bulid && vite build --ssr.)

This means that the first vite version comes from user land whereas the second vite version comes from this repository's vite-plugin-ssr's vite devDependency.

I'm surprised though because I believe that vite-ecocystem-ci uses pnpm.overrides.vite, so I'm not sure why there can be two different installed versions then.

Preact now supports Vite 3

Neat. I re-enabled Preact's test suite. I also re-enabled urql's test suite which now also works for Vite 3. Everything is green and no test suites are being skipped anymore. 🎉

Why did vite-ecocystem-ci failed when vite3 was green?

I wonder why that is since I was under the impression that the only thing vite-ecocystem-ci does is to use pnpm.overrides, which is exactly what the vite3 branch does. There are several instances where vite3 was green whereas vite-ecocystem-ci failed.

So it seems that vite-ecocystem-ci does some other things, which I'm not sure it's something we want since it will lead to other PRs like this one. (I did regularly rebased vite3 on top of main to ensure that vite-ecocystem-ci stays green.)

@sapphi-red sapphi-red deleted the for-ecosystem-ci branch July 24, 2022 11:14
@sapphi-red
Copy link
Contributor Author

I'm surprised though because I believe that vite-ecocystem-ci uses pnpm.overrides.vite, so I'm not sure why there can be two different installed versions then.

I meant this repository uses two different versions and the vite-ecosystem-ci uses the same version. I now understand it's because of the devDependency of vite-plugins-ssr.

I re-enabled Preact's test suite. I also re-enabled urql's test suite which now also works for Vite 3. Everything is green and no test suites are being skipped anymore. 🎉

🎉

Why did vite-ecocystem-ci failed when vite3 was green?

I found that vite-ecosystem-ci uses ni to detect the package manager. ni uses packageManager field.
So ecosystem-ci used pnpm@6 previously.
But this repository explicitly installs pnpm@7. Since this repository has packageManager field, it might be better to remove with.version here, because packageManager field will be used if it is omitted.

BTW I think this needs to be GITHUB_REPOSITORY instead of GIT_REPOSITORY. (I'm not sure why it isn't failing in this repo.)
https://github.com/sapphi-red/vite-ecosystem-ci/runs/7487536712?check_suite_focus=true#step:7:443

@brillout
Copy link
Member

it might be better to remove with.version here

👍 Done: b8060ac.

BTW I think this needs to be GITHUB_REPOSITORY instead of GIT_REPOSITORY. (I'm not sure why it isn't failing in this repo.) https://github.com/sapphi-red/vite-ecosystem-ci/runs/7487536712?check_suite_focus=true#step:7:443

It's set here: https://github.com/brillout/vite-plugin-ssr/blob/367a5b2a836cf43d1187074f4c1900c0abcd0c0e/.github/workflows/ci.yml#L68

Is there a way to detect whether the test is being run in vite-ecosystem-ci? I can then change the code accordingly.

@sapphi-red
Copy link
Contributor Author

It's set here:
Is there a way to detect whether the test is being run in vite-ecosystem-ci? I can then change the code accordingly.

I see. I think just replacing GIT_REPOSITORY with GITHUB_REPOSITORY will work. GITHUB_REPOSITORY is automatically defined by GitHub Actions.
https://docs.github.com/en/actions/learn-github-actions/environment-variables#default-environment-variables

@brillout
Copy link
Member

Done 9e3ad05. I also think it should work now 👍.

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.

2 participants