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

feat: add comment directive to ignore style collection #324

Merged
merged 1 commit into from
Jul 10, 2024

Conversation

XiNiHa
Copy link
Contributor

@XiNiHa XiNiHa commented Jul 7, 2024

Continued from #316, cc @katywings

This PR adds // @vinxi-ignore-style-collection directive to ignore a module from the style collection.
The directive only works when added at the top of the file, before having any meaningful code beside comments and whitespaces.

Copy link

codesandbox bot commented Jul 7, 2024

Review or Edit in CodeSandbox

Open the branch in Web EditorVS CodeInsiders

Open Preview

Copy link

changeset-bot bot commented Jul 7, 2024

⚠️ No Changeset found

Latest commit: 1d85e5d

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link

vercel bot commented Jul 7, 2024

@XiNiHa is attempting to deploy a commit to the Nikhil Saraf's projects Team on Vercel.

A member of the Team first needs to authorize it.

@katywings
Copy link
Collaborator

@XiNiHa Very nice 🥳, thank you for your work! One question from my side: did you check how much this regex overall degrades the performance of the css collection and if it somehow can be skipped on repeated redundant collections (especially in prod)

@XiNiHa
Copy link
Contributor Author

XiNiHa commented Jul 10, 2024

Not really 😅 I haven't had any concerns around performance since RegExp is mostly fast in JS and I didn't use any performance-critical features like lookahead. Also since it's only a build-time work so just adding a RegExp check wouldn't be that critical to performance (tbh replacing Babel with SWC would make much more impact here)

@ryansolid
Copy link
Collaborator

I mean if we decided to do this I don't see a way of avoiding the reading through the code like this. So we are going to give it a shot. If it slows things down considerably we would need to adjust it minorly or I suppose come up with a different solution. But I think we are sort of priced into something like this with the agreed on solution.

@ryansolid ryansolid merged commit 9c33c0b into nksaraf:main Jul 10, 2024
1 of 6 checks passed
@XiNiHa XiNiHa deleted the feat/ignore-css-collection branch July 11, 2024 02:58
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.

3 participants