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(deps): update dependency @rollup/plugin-commonjs to v22 #7894

Closed

Conversation

NMinhNguyen
Copy link
Contributor

Description

Additional context


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the Commit Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@fwouts
Copy link
Contributor

fwouts commented May 18, 2022

@NMinhNguyen Are you still working on this PR?

@NMinhNguyen
Copy link
Contributor Author

@NMinhNguyen Are you still working on this PR?

Not actively, no, sorry. Feel free to pick up where I left off

@fwouts
Copy link
Contributor

fwouts commented May 18, 2022

@NMinhNguyen Are you still working on this PR?

Not actively, no, sorry. Feel free to pick up where I left off

Thanks for the quick reply.

It seems we need to follow the advice given here to make it all work again, but I'm completely unfamiliar with this area of the codebase so I'd have a difficult time taking this on.

@patak-dev
Copy link
Member

Interesting, test-serve works fine locally for me. This should fix the build errors 8b43995

@patak-dev
Copy link
Member

@fwouts did you have other fixes for the serve tests that aren't in that branch? The one there doesn't seem to make a difference. It is strange, maybe this is something with the rollup version? The commonjs plugin shouldn't affect serve.

@fwouts
Copy link
Contributor

fwouts commented May 18, 2022

@fwouts did you have other fixes for the serve tests that aren't in that branch? The one there doesn't seem to make a difference. It is strange, maybe this is something with the rollup version? The commonjs plugin shouldn't affect serve.

Here's my old PR attempt, I think it might be this particular line: https://github.com/vitejs/vite/pull/7892/files#diff-5ac5675edd621a639999ba602b8795d985ca4bc7c9098473e6692d04e34291d9

Sorry I don't have much time to dive deeper into this for the next couple of weeks!

@patak-dev
Copy link
Member

I don't think that change is ok @fwouts, as it will catch empty code too ''. And don't worry, no rush. I can't reproduce the CI issues locally though, the PR is working in my env.

@patak-dev patak-dev added the p2-nice-to-have Not breaking anything but nice to have (priority) label May 19, 2022
@patak-dev
Copy link
Member

nr build vite core, then test-serve fails locally too
nr dev vite core, then test-serve works without problems 🤔

@bluwy was proposing we may want to look into using esbuild during build for CJS, as it was proposed also by Evan before

@fwouts
Copy link
Contributor

fwouts commented May 19, 2022

@bluwy was proposing we may want to look into using esbuild during build for CJS, as it was proposed also by Evan before

I do agree this is the better solution in the longer term. We'll be much less likely to see build-specific issues. If this could be tackled for Vite 3, it would be amazing!

@sapphi-red
Copy link
Member

I've add a commit which excludes ts-node from bundle. I don't understand why it started to be bundled.🤔
But this fixes test-serve.
I tested updating plugin-node-resolve but it did not fix this.

Also when I don't set minThreads/maxThreads to 1, test-build finishes incompletely without any error messages. It only shows ELIFECYCLE  Command failed with exit code 1.. I'm using windows10+node 16.15.0.

@patak-dev
Copy link
Member

I do agree this is the better solution in the longer term. We'll be much less likely to see build-specific issues. If this could be tackled for Vite 3, it would be amazing!

Testing appreciated 😉

@sapphi-red
Copy link
Member

Closing as #8743 was merged.

@sapphi-red sapphi-red closed this Jun 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2-nice-to-have Not breaking anything but nice to have (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants