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

Rewrite @import paths #34

Merged
merged 4 commits into from
Jul 16, 2017
Merged

Conversation

coditect
Copy link

@coditect coditect commented May 8, 2017

The documentation for sass-resources-loader makes it quite clear that @import rules are not allowed in resource files. However, I have found them to be unavoidable when working with Foundation for Sites. My changes give sass-resources-loader the ability to rewrite the paths of any @import rules found in resource files so that sass-loader can resolve those imports later in the build process.

Potential issues:

  • No attempt it made to escape disallowed characters in the rewritten import paths.
  • Not compatible with import directives in LESS. Workaround provided in 6582c4e.

This change is Reviewable

@justin808
Copy link
Member

@coditect FABULOUS! I'll review (and merge) once you

  • Add docs
  • Add example verifying this work
  • Add CHANGELOG entry to your PR.

@coditect
Copy link
Author

@justin808 I added docs and an example as requested. Anything else?

@alex35mil
Copy link
Member

@coditect The initial reason why imports are not allowed is that it slows down incremental builds significantly (see #5). I assume this issue still persist w/ this change, so I'd add to the docs the reason why @imports should be avoided.

The rest LGTM, thanks for doing this!

@justin808
Copy link
Member

@alexfedoseev are you going to merge and update the npm library?

@justin808
Copy link
Member

@alexfedoseev want me to merge and update NPM?

@alex35mil alex35mil merged commit df959b5 into shakacode:master Jul 16, 2017
@IAMtheIAM
Copy link

Please release via NPM

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.

None yet

4 participants