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[Tailwind]: Correct content purge paths #2

Closed
wants to merge 1 commit into from

Conversation

benoror
Copy link

@benoror benoror commented Aug 25, 2022

Closes #1

@awesomeunleashed
Copy link

awesomeunleashed commented Aug 25, 2022

Hey @benoror,

This issue is actually by design, sort of. When we switched to using a fork, I wanted to keep our existing import {} from @windmill/react-ui statements intact in case the original library was ever updated.

So, to do that and fix the issue you're seeing, we do this in package.json dependencies:

"@windmill/react-ui": "npm:@cast-corp/windmill-react-ui@^0.7.4",

Doing it that way, the package is still installed in the node_modules/@windmill/react-ui folder, making the original imports and purge paths correct.

I think there are two reasonable options here:

  1. Leave it as-is (other than that purge->content fix) and make a note in the README about this, or
  2. Include both sets of purge paths so that the package can be installed either way.

Thoughts?

@benoror
Copy link
Author

benoror commented Aug 25, 2022

@awesomeunleashed ahhh, makes total sense!

I actually like the idea of preserving that backwards-compatibility with the OG package (although I am less zero hopeful there's actually gonna be a new version coming out 😅 )

I'll like option 1., can make changes to README and leave the content fix (although actually the merge would not interfere with the existing purge key tbh)

I also like option 2. as that won't be an issue for newcomers who skip the aliasing in package.json

How about the two of them? I can work on the changes

@benoror
Copy link
Author

benoror commented Aug 25, 2022

@awesomeunleashed ok here's what I tried:

  1. Made your suggested alias change in package.json:
--   "@cast-corp/windmill-react-ui": "0.7.4",
++   "@windmill/react-ui": "npm:@cast-corp/windmill-react-ui@^0.7.4",
  1. Changed all instances of
-- import { ... } from "@cast-corp/windmill-react-ui"
++ import { ... } from "@windmill/react-ui"

including this require from tailwind.config.js

-- const windmill = require('@cast-corp/windmill-react-ui/config')
++ const windmill = require('@windmill/react-ui/config')
  1. And finally removed the content purge lines:
  content: [
    "./pages/**/*.{js,ts,jsx,tsx}",
    "./components/**/*.{js,ts,jsx,tsx}",
--    "../../node_modules/@cast-corp/windmill-react-ui/lib/defaultTheme.js",
--    "../../node_modules/@cast-corp/windmill-react-ui/dist/index.js",
  ],

Unfortunately I get the same styling issue afterwards (in next.js production build), if I bring the content-purge lines back (with corrected alias path) it seems to work well again:

  content: [
    "./pages/**/*.{js,ts,jsx,tsx}",
    "./components/**/*.{js,ts,jsx,tsx}",
++    "../../node_modules/@windmill/react-ui/lib/defaultTheme.js",
++    "../../node_modules/@windmill/react-ui/dist/index.js",
  ],

🤔 Wondering what it could be then?

@awesomeunleashed
Copy link

🤔 Wondering what it could be then?

Hmm. It does sound like that should have worked. I'll verify the classes are actually purged correctly from those files in our project (we've overridden almost all of the default theme, so it might be possible that it actually hasn't been working but everything was just being picked up from the custom theme).

including this require from tailwind.config.js

-- const windmill = require('@cast-corp/windmill-react-ui/config')
++ const windmill = require('@windmill/react-ui/config')

Just to be sure we're looking at the same thing, your tailwind.config.js looks something like this, correct?

const windmill = require("@windmill/react-ui/config");

module.exports = windmill({
    // content, darkMode, plugins, theme, etc.
});

@benoror
Copy link
Author

benoror commented Aug 25, 2022

Just to be sure we're looking at the same thing, your tailwind.config.js looks something like this, correct?

yup, here's is it:

const defaultTheme = require('tailwindcss/defaultTheme');
const windmill = require('@windmill/react-ui/config')

module.exports = windmill({
  content: [
    "./pages/**/*.{js,ts,jsx,tsx}",
    "./components/**/*.{js,ts,jsx,tsx}",
    "../../node_modules/@windmill/react-ui/lib/defaultTheme.js",
    "../../node_modules/@windmill/react-ui/dist/index.js",
  ],
  theme: { ...  },
  plugins: [],
})

@awesomeunleashed
Copy link

awesomeunleashed commented Aug 25, 2022

    "../../node_modules/@windmill/react-ui/lib/defaultTheme.js",
    "../../node_modules/@windmill/react-ui/dist/index.js",

Perhaps it could be because (I assume based on these paths) your node_modules isn't in the same folder as the tailwind.config.js? I haven't tested it, but if the paths currently in config.js are relative to tailwind.config.js, then I bet they wouldn't work for you even if I merged this PR.

If that is the issue, then those lines would have to be updated to always find node_modules correctly in order for them to work for your repo setup.

@benoror
Copy link
Author

benoror commented Aug 25, 2022

Perhaps it could be because (I assume based on these paths) your node_modules isn't in the same folder as the tailwind.config.js?

That was one of my suspicions before (#1 (comment)), let me try again with this new context

@benoror
Copy link
Author

benoror commented Aug 25, 2022

@awesomeunleashed It did work! I assume postcss is not smart enough to deduce paths for workspaces during the build phase. I'll keep those two purge lines as a reminder.

Thinking it twice now I don't think many people would face this issue unless they have an exotic folder structure, so I am ok closing this if you're

@awesomeunleashed
Copy link

Great, glad you got it working. I'm guessing there's a way to make those paths search for the correct node_modules path instead of being hardcoded, but if you're satisfied with your workaround, I'll close this.

I did add installation notes to the README with a summary of the relevant information from our discussions.

@benoror
Copy link
Author

benoror commented Aug 26, 2022

@awesomeunleashed you're great, thanks so much for your time!

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.

Styles not purged in Next.js
2 participants