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

Resolve relative paths for preprocessor styles #5829

Merged
merged 1 commit into from
Aug 6, 2019
Merged

Resolve relative paths for preprocessor styles #5829

merged 1 commit into from
Aug 6, 2019

Conversation

iamandrewluca
Copy link
Contributor

@iamandrewluca iamandrewluca commented Nov 16, 2018

preprocessor will output sourceMap by default
then check if sourceMap are needed on resolve-url-loader

issue demo: https://github.com/iamandrewluca/cra-scss-relative-problem

image

Fixes #4653

@stale
Copy link

stale bot commented Dec 16, 2018

This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed in 5 days if no further activity occurs.

@stale stale bot added the stale label Dec 16, 2018
@iamandrewluca
Copy link
Contributor Author

@gaearon @Timer any fedback on this please? :)

@stale stale bot removed the stale label Dec 17, 2018
@Timer
Copy link
Contributor

Timer commented Dec 23, 2018

Sorry this fell through the cracks! I'm going to tag it for a milestone and hopefully get back to it soon. 😄

@Timer Timer added this to the 2.1.x milestone Dec 23, 2018
@yazeedb
Copy link

yazeedb commented Jan 16, 2019

I ran this change on an internal TypeScript app, and can confirm it resolves the issue.

Might fork react-scripts to use it for now, but getting this merged would be sweeeeeeet.

@Silic0nS0ldier
Copy link

Hi everyone, any progress on this? Would be nice to clear out the dependency resources that have had to be moved into a project I'm working on. However the longer this goes unfixed, the less likely that is to happen.

@iamandrewluca
Copy link
Contributor Author

Rebased

iamandrewluca added a commit to roataway/roataway-web that referenced this pull request Mar 8, 2019
Revert eslint changes when facebook/create-react-app#6513
will be merged

Revert to react-scripts when facebook/create-react-app#5829
will be merged
@iamandrewluca
Copy link
Contributor Author

  • Rebased.
  • Updated resolve-url-loader to last version.

@iansu iansu modified the milestones: 2.1.x, 3.x Mar 10, 2019
@jackwilsdon
Copy link
Contributor

This seems to have issues when using NODE_PATH, as the path resolves to a relative path outside of src;

src/index.scss

@import '~example/scss/index.scss';

vendor_modules/example/scss/index.scss

body {
  background: url('../logo.svg');
}

Output from yarn build with NODE_PATH=./vendor_modules;

ModuleNotFoundError: Module not found: Error: You attempted to import ../vendor_modules/example/logo.svg which falls outside of the project src/ directory. Relative imports outside of src/ are not supported.
You can either move it inside src/, or add a symlink to it from project's node_modules/.

@iamandrewluca
Copy link
Contributor Author

iamandrewluca commented Mar 20, 2019

@jackwilsdon this is not related to current issue. CRA has this restrictions from the beginning. If CRA would allow relative sources that fall outside src this error should not happen.

@jackwilsdon
Copy link
Contributor

My apologies @iamandrewluca, I misunderstood this PR as being a potential fix for #5402, which is caused by sass-loader not knowing the full path to the file and instead assuming it's not in NODE_PATH or src.

@iamandrewluca
Copy link
Contributor Author

@jackwilsdon no problem. I added a reference in #5402 (comment)

@DalerAkhmetov
Copy link

thank you for this pull request. It also be useful for working with Font Awesome 5 icon library.

There is some code in SASS file located, for example, in '/app/src/styles.scss':
@import "~@fortawesome/fontawesome-free/scss/fontawesome"; @import "~@fortawesome/fontawesome-free/scss/solid"; // this file contains "url('#{$fa-font-path}/fa-solid-900.eot')"
This sass file will generate error: Module not found: Can't resolve '../webfonts/fa-solid-900.eot' in '/app/src'.

@mrmckeb mrmckeb self-assigned this Jun 25, 2019
@mrmckeb
Copy link
Contributor

mrmckeb commented Jun 25, 2019

Hi all, sorry this fell through the cracks. I'll follow this up for now on.

@iamandrewluca - are you still interested in working on this? If so, let me know and I'll work with you to get it in.

@iamandrewluca
Copy link
Contributor Author

@mrmckeb yes, still interested.

Copy link
Contributor

@mrmckeb mrmckeb left a comment

Choose a reason for hiding this comment

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

OK, great. Let me take a look at this over the next few days.

I can see that you've included a demo which is great.

One small comment, otherwise it looks good.

packages/react-scripts/config/webpack.config.js Outdated Show resolved Hide resolved
@iamandrewluca
Copy link
Contributor Author

@mrmckeb done!

Copy link
Contributor

@mrmckeb mrmckeb left a comment

Choose a reason for hiding this comment

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

@ianschmitz or @iansu - any concerns on this one?

@mrmckeb mrmckeb modified the milestones: 3.x, 3.1 Jul 24, 2019
@mrmckeb
Copy link
Contributor

mrmckeb commented Aug 5, 2019

Still waiting for feedback on this one, but I'd like to get it out in 3.1. @ianschmitz was away, but hopefully he can take a look this week.

Copy link
Contributor

@ianschmitz ianschmitz left a comment

Choose a reason for hiding this comment

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

Just a quick change otherwise looks good.

packages/react-scripts/package.json Outdated Show resolved Hide resolved
preprocessor will output sourceMap by default
then check if sourceMaps are needed on resolve-url-loader

Fixes #4653
@ianschmitz ianschmitz merged commit 914c95e into facebook:master Aug 6, 2019
@lock lock bot locked and limited conversation to collaborators Aug 11, 2019
@iamandrewluca iamandrewluca deleted the resolve-url-loader branch December 5, 2019 20:23
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot resolve relative references in nested .scss files with react-scripts@2.0
10 participants