Skip to content

refactor: importAnalysisBuild #19723

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

privatenumber
Copy link
Contributor

Description

Pulling out some minor refactors from #19722

@privatenumber privatenumber force-pushed the refactor-import-analysis-build branch from 154314e to 4098d6f Compare March 26, 2025 04:57
@privatenumber privatenumber marked this pull request as ready for review March 26, 2025 05:04
@privatenumber privatenumber force-pushed the refactor-import-analysis-build branch 3 times, most recently from e5912cc to 0d85f7d Compare March 26, 2025 06:08
@sapphi-red sapphi-red added the p1-chore Doesn't change code behavior (priority) label Mar 27, 2025
@privatenumber privatenumber force-pushed the refactor-import-analysis-build branch 6 times, most recently from 309a0f4 to aaa2273 Compare March 27, 2025 13:00
@privatenumber privatenumber force-pushed the refactor-import-analysis-build branch 8 times, most recently from 62ce4aa to 0325471 Compare March 28, 2025 08:24
sapphi-red
sapphi-red previously approved these changes Mar 28, 2025
@privatenumber privatenumber force-pushed the refactor-import-analysis-build branch 2 times, most recently from 0958fd6 to 66cd77e Compare March 28, 2025 10:33
@privatenumber
Copy link
Contributor Author

With this refactor, I also have a follow up perf optimization to de-duplicate the chunk registry:
#19757

@privatenumber privatenumber force-pushed the refactor-import-analysis-build branch from 66cd77e to 2496007 Compare March 31, 2025 03:46
@privatenumber
Copy link
Contributor Author

@sapphi-red Any updates on this?

Just so you know, this is a refactor as part of a larger fix in #19722.
All tests are passing, and it fixes a fundamental bug in how Vite patches dynamic imports.

Would love to get these in before it becomes outdated.

@sapphi-red
Copy link
Member

We normally require 2 or more reviews to merge a PR, so another review is needed. Let me add this to the team board so that I can discuss in the team meeting and speed up the process.

@sapphi-red sapphi-red added p2-to-be-discussed Enhancement under consideration (priority) and removed p1-chore Doesn't change code behavior (priority) labels Apr 15, 2025
@github-project-automation github-project-automation bot moved this to Discussing in Team Board Apr 15, 2025
@sapphi-red sapphi-red moved this from Discussing to P2 - 3 in Team Board Apr 15, 2025
@sapphi-red
Copy link
Member

First of all, thank you for your work on these pull requests.

In our last team meeting, we discussed refactoring PRs and established a policy for coding style changes (see contribution guide change). Even if we also agree some of these PRs have value, they also make git history more difficult to navigate and potentially introduce subtle issues. In general, we'd like to only merge refactors when they are required to implement features or bug fixes.

So we prefer not to merge this specific PR as it primarily contains style adjustments. However, we'd still love to merge the other dependent PRs if you'd like to rebase those onto main.

Thanks!

@sapphi-red sapphi-red moved this from P2 - 3 to Has plan in Team Board Apr 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2-to-be-discussed Enhancement under consideration (priority)
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants