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: attempt to fix sass resolving root of package instead of subfolder #18135

Closed
wants to merge 2 commits into from

Conversation

HolyNoodle
Copy link

Description

This PR attempts to fix the issue seen in this issue: #18134

Currently, the process for resolving SASS files is looking at the root of the directory and fails if the build files are in a subfolder.

Copy link

stackblitz bot commented Sep 18, 2024

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@HolyNoodle HolyNoodle changed the title attempt to fix: sass resolving root of package fix: attempt to fix sass resolving root of package Sep 18, 2024
@HolyNoodle HolyNoodle changed the title fix: attempt to fix sass resolving root of package fix: attempt to fix sass resolving root of package instead of subfolder Sep 18, 2024
@hi-ogawa
Copy link
Collaborator

Thanks for the PR. I'm checking locally and this seems to technically break if a package has some-package/index.scss but main js entry is in a different directory like some-package/js/index.js:

{
  "name": "some-package",
  "exports": "./js/index.js"
}

I'm not sure about the history, but it looks like this sass specific logic is initially introduced in #3627 and probably the intent is to always check some-package/index.scss (or _index.scss) regardless of main entry.

Looking at your reproduction in #18134, you can fix this specific resolution error by explicitly adding sass export entry. Would something like this work for you?

{
  "name": "lib",
  "version": "1.0.0",
  "main": "./build/index",
  "module": "./build/index.esm.js",
  "exports": {
    ".": {
      "sass": "./build/index.scss",
      "import": "./build/index.esm.js",
      "require": "./build/index.js"
    }
  }
}

@HolyNoodle
Copy link
Author

HolyNoodle commented Sep 18, 2024

@hi-ogawa Thank you for your answer. This is promising but my current test still show the same error on the reproduction package in the issue.

I agree that this solution isn't going to work for every situation. But the current solution works only for if the scss file is at the root of the package folder and the JS files can be placed in anywhere.

@HolyNoodle
Copy link
Author

My bad, I had to make it the first property. It fixes the reproduction example. I close this PR.

Thank you for your time. I'll update the issue so if someone has the same issue they have the solution right away

@HolyNoodle HolyNoodle closed this Sep 18, 2024
@HolyNoodle
Copy link
Author

Just out of my mind, having a log stating which file it was trying to resolve with which context could be interesting for debugging as it helps pinpointing what is the problem and as such detect a problem in configuration.

I am carrying on the migration but I added a log at a place so I can see at failure what it was trying to do. Very hepful but not an easy task for everyone.

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