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

build: improve css exports in package.json #2547

Merged
merged 4 commits into from
Oct 17, 2024
Merged

Conversation

karlshea
Copy link
Contributor

Pull Request Template

Thanks for your PR! Make sure you have read the CONTRIBUTING guide.

What's Changed

Briefly describe the changes in this pull request.

Type of Change

  • Bug fix
  • New feature
  • Breaking change
  • Documentation update

Tips for a good PR

  • If you are changing code, please add tests to cover the changes.
  • Some screenshots or screen recording could help to understand the changes.
  • If it is a bug, please provide the code to reproduce it.

Thanks

Additional Notes

Test PR updating stylesheet exports to fix SASS file resolution using sass-loader and webpack.

@gpbl gpbl self-requested a review October 16, 2024 15:05
@karlshea
Copy link
Contributor Author

Is the build-website fail after you pushed up the change?

It's crazy that it's impossible to find decent docs about how module resolution is supposed to work. sass-loader says it uses webpack's resolution and they have docs here: https://webpack.js.org/configuration/resolve/

According to those what you originally had should have worked. I'll open an issue with sass-loader and see if they have any input.

@karlshea
Copy link
Contributor Author

Discussion link here: webpack/webpack#18866

@karlshea
Copy link
Contributor Author

Got an answer in that discussion, sass-loader checks a different key!

@karlshea
Copy link
Contributor Author

Oh I see you fixed the import. So I guess it could go either way, the change I just pushed to add style/import/require or we could revert it back to just listing out default/types without the nested keys. Either seems to be working when I run my build.

@gpbl
Copy link
Owner

gpbl commented Oct 17, 2024

@karlshea yes I've patched your PR with some more changes.

Oh I see you fixed the import. So I guess it could go either way, the change I just pushed to add style/import/require or we could revert it back to just listing out default/types without the nested keys. Either seems to be working when I run my build.

I won't touch it anymore :D thanks for your help here hopefully will help your work.

BTW the TypeScript exports are now:

Screenshot 2024-10-16 at 11 26 40 AM

@gpbl gpbl changed the title Remove nested import/require from stylesheet exports build: improve css exports in package.json Oct 17, 2024
@gpbl gpbl merged commit f2f737e into gpbl:main Oct 17, 2024
10 checks passed
@gpbl gpbl removed their request for review October 17, 2024 12:52
@karlshea
Copy link
Contributor Author

Thanks for taking a look! Also would like to say thanks for the package, I spent the last several days moving a big project from moment/react-datetime to date-fns/react-day-picker and it's been very nice to work with!

@karlshea karlshea deleted the style-exports branch October 17, 2024 16:18
@gpbl
Copy link
Owner

gpbl commented Oct 17, 2024

Thanks for taking a look! Also would like to say thanks for the package, I spent the last several days moving a big project from moment/react-datetime to date-fns/react-day-picker and it's been very nice to work with!

Thank you for your kind words and support! If you have the time, we’d love for you to share your experiences or any suggestions on how we can further improve the library.

@gpbl
Copy link
Owner

gpbl commented Oct 17, 2024

(will publish the fix later in the day...)

@karlshea
Copy link
Contributor Author

The only thing I ran into is actually #2549 and then saw there was an open issue for it already, but it's not a huge blocker or anything. Everything else just worked how I would have hoped!

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.

2 participants