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(plugin-react): process excluded files (fix #6921) #8758

Closed
wants to merge 1 commit into from

Conversation

ZYSzys
Copy link
Contributor

@ZYSzys ZYSzys commented Jun 24, 2022

Description

Do filter(id) on top of transform function to exclude files.

Fixes vitejs/vite-plugin-react#24

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 vitejs/vite#123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@netlify
Copy link

netlify bot commented Jun 24, 2022

Deploy Preview for vite-docs-main canceled.

Name Link
🔨 Latest commit ee4e29b
🔍 Latest deploy log https://app.netlify.com/sites/vite-docs-main/deploys/62b59778b60c1a00094904bf

@sapphi-red
Copy link
Member

I think this is a breaking change as the behavior is documented that include/exclude could be used to filter hmr enabled files.
https://github.com/vitejs/vite/tree/main/packages/plugin-react#filter-which-files-use-fast-refresh

I'm not much familiar with react, but I suppose adding a different property like includeTransform is better.

@bluwy
Copy link
Member

bluwy commented Jun 24, 2022

I think this PR's way of using include/exclude make sense, and is usually how Rollup plugins work. Perhaps we can take the beta opportunity to make a breaking change. And for adjusting include/exclude for fast refresh, we can overload the fastRefresh option as { include, exclude } | boolean

@patak-dev
Copy link
Member

Sounds good to me. @aleclarson ping so you're aware too. Let us know if you have an issue with the change.

@sapphi-red
Copy link
Member

sapphi-red commented Jun 25, 2022

Maybe this one is doing slightly similar? #6202

@aleclarson
Copy link
Member

Yeah I think #6202 is a better approach. Someone should write tests for it 😁

@ZYSzys
Copy link
Contributor Author

ZYSzys commented Jun 27, 2022

Yeah I think #6202 is a better approach. Someone should write tests for it 😁

@aleclarson Yeah, I'm willing to write some tests for #6202, but I found there are some conflicts need to be resolved, could you make a git rebase from main branch first ?

@aleclarson
Copy link
Member

aleclarson commented Jun 28, 2022

@ZYSzys I will rebase it sometime in the next few days, thanks!

@patak-dev
Copy link
Member

Closing this PR as #6202 seems like a better solution for the folks involved. @ZYSzys if you are going to add tests, you could fork @aleclarson PR, rebase, and push it yourself to the new plugin-react repository.

@patak-dev patak-dev closed this Dec 3, 2022
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.

@vitejs/plugin-react processing excluded files
5 participants