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: css imports with extension were not bundled in output. #55

Merged
merged 4 commits into from
Aug 17, 2018

Conversation

arpit-agarwal
Copy link
Contributor

The PR fixes #48 by looking for valid extensions before attaching default extension.

I used tar-ball of this PR and fixed #48 in a branch
git clone -b issue/48 https://github.com/arpit-agarwal/scss-bundle-issues.git
run npm install && npm start.

Theme file is generated as expected on destination location

src/bundler.ts Outdated
@@ -8,7 +8,8 @@ import * as Helpers from "./helpers";
const IMPORT_PATTERN = /@import\s+['"](.+)['"];/g;
const COMMENT_PATTERN = /\/\/.*$/gm;
const MULTILINE_COMMENT_PATTERN = /\/\*[\s\S]*?\*\//g;
const FILE_EXTENSION = ".scss";
const DEFAULT_FILE_EXTENSION = ".scss";
const ALLOWED_FILE_EXTENSIONS = [".scss", ".saas", ".css"];
Copy link
Member

Choose a reason for hiding this comment

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

Does it really work with .sass?

Copy link
Member

Choose a reason for hiding this comment

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

I tried https://www.sassmeister.com/ with this snippet and I failed:

.test {
  color: red;
}

nav
  ul
    margin: 0
    padding: 0
    list-style: none

  li
    display: inline-block

  a
    display: block
    padding: 6px 12px
    text-decoration: none

Copy link
Member

@MartynasZilinskas MartynasZilinskas left a comment

Choose a reason for hiding this comment

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

Waiting to resolve comments.

@arpit-agarwal
Copy link
Contributor Author

arpit-agarwal commented Aug 17, 2018

Good catch. I just renamed a scss file and tested 😞

Dropped the saas from allowed extension list now.

@lazarljubenovic
Copy link

It was a typo, it used to say saas instead of sass. Not sure if that influenced the errors?

@MartynasZilinskas
Copy link
Member

@lazarljubenovic What do you mean?

@lazarljubenovic
Copy link

image

The code said .saas and based on that it was determined that Sass .sass doesn't work.

@arpit-agarwal
Copy link
Contributor Author

arpit-agarwal commented Aug 17, 2018

@lazarljubenovic I just tested this will not work with sass ext as well. The code inside bundler is suggesting some token to be invalid.

@MartynasZilinskas Please move ahead with merge.

@MartynasZilinskas
Copy link
Member

🎉

@MartynasZilinskas MartynasZilinskas merged commit 3192f0e into reactway:master Aug 17, 2018
@MartynasZilinskas
Copy link
Member

v2.4.0 is released 😄.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Issues Importing .css file from node_modules
3 participants