-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Tailwind Setup: Use fully specified paths #1648
Conversation
|
Fixing this uncovered another bug with the tailwind setup.
I'll try to fix that once this PR is merged |
@Tobbe please do take a look at the original PR addressing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Let me know if you need a Mac use to test first before merging. Otherwise 🚀
That would be great! This is how I tested it:
|
I went back and took another look at this. What a mess the tailwind installation is! I have two installations! One in ~/.../twpaths/node_modules and another one in ~/.../twpaths/web/node_modules So if I'm running the setup command in ~/.../twpaths it will use the installation in ~/.../twpaths/node_modules, and files will be created relative to that path. If I'm somewhere inside web/, like ~/.../twpaths/web/src/pages, it will use the installation in ~/.../twpaths/web/node_modules and files will be created relative to that path instead. |
1f12f1d
to
b45bad0
Compare
You mean this one? #1301 I don't see anything in there about blocking Here's my understanding of the issue I found:Scenario 1 (works)
At this point, if you were to run Our setup command moves the config file to
Scenario 2 (fails)
At this point, if you were to run Our setup command moves the config file to
|
a94040c
to
dab69e8
Compare
@thedavidprice If you could run a quick test on your (mac) computer, that would be awesome!
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Tobbe this is working (almost) 💯 on Mac! From root, install and --force work correctly. From a sub-directory, install works correctly but --force runs into an issue with Tailwind yarn tailwindcss init
that fails if tailwind.config.js
exists. This is the reason why we added the "Sync ..." step that runs yarn install --check-files
, which avoids Tailwind throwing. Here's the error:
Command failed with exit code 1: yarn tailwindcss init
🚫 tailwind.config.js already exists.
error Command failed with exit code 1.
$ /Users/price/Repos/xx-delete/node_modules/.bin/tailwindcss init
@tailwindcss/postcss7-compat 2.0.2
To run --force successfully from any directory, I think we need to set the Execa working directory to root on this line:
await execa('yarn', ['install', '--check-files']) |
@Tobbe Thinking out loud here, maybe we can pass |
Thanks for the thorough testing. You're seeing the exact same thing as I'm seeing on Windows. You can compare your output with what I posted in a previou comment #1648 (comment) As I also said in that comment, my plan was to merge this fix, and then write a separate fix for the --force stuff. But I guess I can do it all here. My plan for fixing --force was to just manually delete
|
Ah, roger that @Tobbe I (falsely) assumed you were ready to go and skipped over:
Yes, |
I don't think that's true. The reason it works from root is that there is no tailwind.config.js file there that
I will. But I don't think it'll help with |
2a4bd73
to
17194f6
Compare
@thedavidprice As I expected, now, when I specify cwd to be web/ it doesn't even work with --force in root anymore, because now
|
17194f6
to
b4398ac
Compare
Now that I delete the config file first it works
|
Next problem to handle: Paths with spaces!
|
b4398ac
to
b8edd16
Compare
🎉
Works with spaces in the path, and both at root level and deep inside web/ |
Ah, understood. I was suggesting to set the cwd to be root. All good. There were two reasons (if I recall correctly) why we had to run the Testing now. |
Testing looks good ✅ @Tobbe We don't need this step anymore. Can you remove starting L82?
|
@thedavidprice I actually already tried that, but just by manually editing the setup command inside node_modules. This is what I got
But since you say it'll work without it I'm going to build a proper new package to test with |
363379a
to
0a20ef9
Compare
Same error with the PR package
|
Super weird. I tested first locally by removing it from my installed package. Well then, let's ship this as you had it! Thanks for trying it out. And, oh my, it's unreal how much time and attention this specific Setup command has required. This is either round 3 or 4 of a deep dive. We must really ❤️ Tailwind... |
Haha. I don't even use tailwind. I just wanted to do the community a service 🤷♂️ I'm running the latest PR package again, on a fresh install, just to rule out my manual changes as the source of the error. Will report back in a bit. |
First run without
|
0a20ef9
to
0a14ca4
Compare
🚀 |
[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [react-player](https://togithub.com/CookPete/react-player) | [`2.12.0` -> `2.13.0`](https://renovatebot.com/diffs/npm/react-player/2.12.0/2.13.0) | [![age](https://developer.mend.io/api/mc/badges/age/npm/react-player/2.13.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/react-player/2.13.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/react-player/2.12.0/2.13.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/react-player/2.12.0/2.13.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | --- ### Release Notes <details> <summary>CookPete/react-player (react-player)</summary> ### [`v2.13.0`](https://togithub.com/CookPete/react-player/blob/HEAD/CHANGELOG.md#v2130) [Compare Source](https://togithub.com/CookPete/react-player/compare/v2.12.0...v2.13.0) - Fix [#​1604](https://togithub.com/CookPete/react-player/issues/1604) - FilePlayer does not work if I passed an array of urls [`#1612`](https://togithub.com/cookpete/react-player/pull/1612) - fix: `src` sttribute become "undefinded" if `url` is an array [`#1648`](https://togithub.com/cookpete/react-player/pull/1648) - Adding keepPlaying to other player types [`#1639`](https://togithub.com/cookpete/react-player/pull/1639) - CI [`#1654`](https://togithub.com/cookpete/react-player/pull/1654) - Swap out broken youtube URL [`#1659`](https://togithub.com/cookpete/react-player/pull/1659) - Add keepPlaying to seekTo [`#1620`](https://togithub.com/cookpete/react-player/pull/1620) - Added forceDisableHls option for FilePlayer [`#1625`](https://togithub.com/cookpete/react-player/pull/1625) - added onPlaybackQualityChange prop [`#1636`](https://togithub.com/cookpete/react-player/pull/1636) - Update the list of supported YouTube domains [`#1599`](https://togithub.com/cookpete/react-player/pull/1599) - Fix [#​1604](https://togithub.com/CookPete/react-player/issues/1604) - FilePlayer does not work if I passed an array of urls ([#​1612](https://togithub.com/CookPete/react-player/issues/1612)) [`#1604`](https://togithub.com/cookpete/react-player/issues/1604) - Support Wisita URLs with query params [`#1591`](https://togithub.com/cookpete/react-player/issues/1591) - Support vimeo manage links [`#1593`](https://togithub.com/cookpete/react-player/issues/1593) - Update readme [`90237f5`](https://togithub.com/cookpete/react-player/commit/90237f51d43fc63870b0e6d0c86f4497f97ca586) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/redwoodjs/redwood). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4zMS41IiwidXBkYXRlZEluVmVyIjoiMzcuMzEuNSIsInRhcmdldEJyYW5jaCI6Im1haW4ifQ==--> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [react-player](https://togithub.com/CookPete/react-player) | [`2.12.0` -> `2.13.0`](https://renovatebot.com/diffs/npm/react-player/2.12.0/2.13.0) | [![age](https://developer.mend.io/api/mc/badges/age/npm/react-player/2.13.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/react-player/2.13.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/react-player/2.12.0/2.13.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/react-player/2.12.0/2.13.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | --- ### Release Notes <details> <summary>CookPete/react-player (react-player)</summary> ### [`v2.13.0`](https://togithub.com/CookPete/react-player/blob/HEAD/CHANGELOG.md#v2130) [Compare Source](https://togithub.com/CookPete/react-player/compare/v2.12.0...v2.13.0) - Fix [#​1604](https://togithub.com/CookPete/react-player/issues/1604) - FilePlayer does not work if I passed an array of urls [`#1612`](https://togithub.com/cookpete/react-player/pull/1612) - fix: `src` sttribute become "undefinded" if `url` is an array [`#1648`](https://togithub.com/cookpete/react-player/pull/1648) - Adding keepPlaying to other player types [`#1639`](https://togithub.com/cookpete/react-player/pull/1639) - CI [`#1654`](https://togithub.com/cookpete/react-player/pull/1654) - Swap out broken youtube URL [`#1659`](https://togithub.com/cookpete/react-player/pull/1659) - Add keepPlaying to seekTo [`#1620`](https://togithub.com/cookpete/react-player/pull/1620) - Added forceDisableHls option for FilePlayer [`#1625`](https://togithub.com/cookpete/react-player/pull/1625) - added onPlaybackQualityChange prop [`#1636`](https://togithub.com/cookpete/react-player/pull/1636) - Update the list of supported YouTube domains [`#1599`](https://togithub.com/cookpete/react-player/pull/1599) - Fix [#​1604](https://togithub.com/CookPete/react-player/issues/1604) - FilePlayer does not work if I passed an array of urls ([#​1612](https://togithub.com/CookPete/react-player/issues/1612)) [`#1604`](https://togithub.com/cookpete/react-player/issues/1604) - Support Wisita URLs with query params [`#1591`](https://togithub.com/cookpete/react-player/issues/1591) - Support vimeo manage links [`#1593`](https://togithub.com/cookpete/react-player/issues/1593) - Update readme [`90237f5`](https://togithub.com/cookpete/react-player/commit/90237f51d43fc63870b0e6d0c86f4497f97ca586) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/redwoodjs/redwood). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4zMS41IiwidXBkYXRlZEluVmVyIjoiMzcuMzEuNSIsInRhcmdldEJyYW5jaCI6Im1haW4ifQ==--> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Fixes #1643