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: normalize path(vitejs/vite#4556) #290

Merged
merged 2 commits into from
Aug 17, 2021

Conversation

ygj6
Copy link
Contributor

@ygj6 ygj6 commented Aug 14, 2021

Fixes vitejs/vite#4556

See issue comment for details

@ygj6 ygj6 requested a review from a team as a code owner August 14, 2021 09:04
@changeset-bot
Copy link

changeset-bot bot commented Aug 14, 2021

🦋 Changeset detected

Latest commit: e733e23

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@vanilla-extract/vite-plugin Patch

Not sure what this means? Click here to learn what changesets are.

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

@ygj6 ygj6 force-pushed the fix-vite-issue-4556 branch from 39a45f1 to 7c396ea Compare August 14, 2021 09:25
Copy link
Contributor

@mattcompiles mattcompiles left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR. Just need you to remove your package.json change.

"@vanilla-extract/integration": "^1.1.0"
},
"devDependencies": {
"@vanilla-extract/integration": "^1.1.0",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure why this was changed? Can you please revert this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/seek-oss/vanilla-extract/blob/7c396ea58f5af60e65176875ae9c93764fe77fd2/packages/vite-plugin/src/index.ts#L2

Since we need to import the normalizePath method and use it, we need to adjust the vite dependencies to dependencies or peerDependencies, otherwise an exception will occur at build.
image

Maybe I should have adjusted it to peerDependencies more appropriately.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. Sorry I misread this change. I think peerDependencies is the right call here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, thank you for the review.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry but you'll need to leave the dev dependency there as well, otherwise it may not be accessible locally.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I don't quite understand what you mean, because it is a plugin for vite, there is no way to run it alone, it can only work with vite, so vitemust exist.

@ygj6 ygj6 force-pushed the fix-vite-issue-4556 branch from 7c396ea to cf6b834 Compare August 16, 2021 01:51
@ygj6
Copy link
Contributor Author

ygj6 commented Aug 16, 2021

I noticed that the check failed and it says I can't modify the dependency, what should I do?
image

@mattcompiles
Copy link
Contributor

@ygj6 Hey Bud. This looks good but it's failing the format check. I tried to push the change to your branch but you haven't given me permission. Running yarn format and pushing the result should be enough to get CI green and then we can merge 👍

@ygj6
Copy link
Contributor Author

ygj6 commented Aug 17, 2021

@ygj6 Hey Bud. This looks good but it's failing the format check. I tried to push the change to your branch but you haven't given me permission. Running yarn format and pushing the result should be enough to get CI green and then we can merge 👍

An invitation has been extended and you need to agree.

@ygj6 ygj6 force-pushed the fix-vite-issue-4556 branch from b6dc9fd to e733e23 Compare August 17, 2021 03:01
@ygj6
Copy link
Contributor Author

ygj6 commented Aug 17, 2021

Done, thanks for your patience in reviewing, I've done yarn formatting of the code.
Also, there were too many commits records and I did a rebase.

Copy link
Contributor

@mattcompiles mattcompiles left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix mate 👍

@mattcompiles mattcompiles merged commit adc1d64 into vanilla-extract-css:master Aug 17, 2021
@seek-oss-ci seek-oss-ci mentioned this pull request Aug 17, 2021
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.

Path includes a dot (.) character
2 participants