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

Add support for Eyeglass #701

Closed
wants to merge 1 commit into from
Closed

Conversation

Snugug
Copy link

@Snugug Snugug commented Sep 16, 2017

#638 was closed by saying "This could have a PR for it…", so I made a PR for it!

Quick explanation of what's happening:

  • Add eyeglass fields to package.json
  • Add keyword field to package.json and include the eyeglass-module keyword
  • Update the files field in package.json to include the requisite eyeglass-exports.js file and the to-sass.js file (more on that in a moment)
  • Add the eyeglass-exports.js file to tell Eyeglass where the files are stored
  • Add to-sass.js file to convert normalize.css in to _normalize.scss after NPM install
  • Add _normalize.scss to git ignore

Let me talk through to-sass.js because it's likely the most controversial of the additions. Sass cannot import raw CSS files, like what is currently distributed. As such, in order to have full compatibility with Eyeglass as expected for Sass authors, we need a Sass file. This is simple enough, change the .css extension to .scss and it can be imported (I also rename it to a Sass partial so it doesn't compile its own file, by prepending _ to the file name). This could be a 1-liner with cp normalize.css _normalize.scss, but then postintall will fail on terminals that don't have a Bash-compatible cp function available. So, I needed to write a tiny file to A) delete _normalize.scss if it's already there and B) replace it with a clean copy based off of normalize.css making sure it's up-to-date. All of this is done with native Node libraries that are available back to version 0.12 of Node, meaning this should work basically everywhere that Node works, and absolutely every version of Node that is not End of Life'd

I have tested installing and using Normalize from my branch in a project I already have set up. Presuming Eyeglass is already set up, using it in a Sass file after running npm install normalize.css --save-dev is @import 'normalize'.


Resolves #638

@Toddses
Copy link

Toddses commented Sep 18, 2017

Sass can import raw CSS files. We import normalize.css on every project just fine.

@Snugug
Copy link
Author

Snugug commented Sep 18, 2017

@Toddses While I have no doubt your setup is able to import CSS files to Sass just fine, it is absolutely not standard behavior for any of the 4 major Sass implementations (Dart Sass, Ruby Sass, libsass, or Node Sass). Even if it were, this PR is specifically addressing an issue that was opened about adding Eyeglass support, which according to that issue the authors weren't against but wanted a PR for. Here's that PR

@Snugug
Copy link
Author

Snugug commented Sep 18, 2017

(in fact, I wrote node-sass-import-once specifically to address CSS not being importable in Sass as a default behavior)

@Toddses
Copy link

Toddses commented Sep 18, 2017

Our setup isn't anything special, pretty common I think. Based on my own searching, libsass supports @import of CSS files as a partial as of a couple years ago (sass/libsass#754). Ruby Sass is promising to support this out-of-the-box in 4.0 (sass/sass#193).

I have no sway over this project, I was just commenting on the "controversial" change as a user.

@jonathantneal
Copy link
Contributor

I think the original request was limited to updating package.json to better accommodate eyeglass, and this is overkill. I would not want this merged.

@necolas
Copy link
Owner

necolas commented Feb 8, 2018

Thanks but this project has never catered to specific tools

@necolas necolas closed this Feb 8, 2018
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.

Any plans to make Normalize eyeglass compatible?
4 participants