Fix #9412 - import Sass dependencies in vendor folder #9458

ncoden · 2016-12-06T05:33:31Z

Previous PR: #9449

Changes:

Revert add normalize dependency #9430: Make normalize-scss a dev dependency.
Copy Sass dependencies to /_vendor/scss and import them in Sass from there.

Why not importing the Sass dependencies as real dependencies and from node_modules:

normalize-scss has a different name between NPM and Bower. 😠
We cannot assume the path to our real dependencies. Old NPM versions store them in our node_modules folder, while newer versions store them in the node_modules folder of the root project.

Previous PR: foundation#9449 Changes: - Revert foundation#9430 - Make `normalize-scss` a dev dependency. - Copy Sass dependencies to `/_vendor/scss` and import them in Sass from there. Why not importing the Sass dependencies as real dependencies and from `node_modules`: we cannot assume the path to our real dependencies. Old npm versions store them in our `node_modules` folder, while newer versions store them in the `node_modules` folder of the root project.

KittyGiraudel · 2016-12-06T12:54:37Z

scss/foundation.scss

@@ -6,10 +6,10 @@
 */

 // Dependencies
-@import "normalize";
+@import "../_vendor/scss/normalize-scss/sass/normalize";


This is a bit of a bummer. Ideally you’d want ../vendor/normalize.

Yes. We could copy only the sass folder to _vendor/ and rename it normalize-scss.
It would give ../_vendor/normalize-scss/normalize.

I think @ncoden's second suggestion makes sense. I don't think we're going to be doing JS in this way (dependency there works a little better, if still pretty poorly), but it still makes sense to scope by package as we may end up with multiple files to selectively import

@kball " second suggestion": do you mean this PR or my previous comment ?

../_vendor/normalize-scss/normalize

oxyc · 2016-12-06T15:07:22Z

My workflow has always consisted of requiring foundation as a dependency in my project and then importing ~foundation-sites/scss/foundation into my sass entry. This PR breaks that setup as the _vendor directory wont exist until gulp builds it. I guess I can live with adding normalize myself and then importing individual component files rather than foundation.scss.
There might be others with this setup though.

ncoden · 2016-12-06T15:26:12Z

@oxyc The _vendor directory is created by the build script before publishing releases to NPM or Bower. If you import foundation-sites from them, the _vendor directory should exists with normalize-scss inside.

oxyc · 2016-12-06T15:27:55Z

@ncoden doesn't the entry in .npmignore prevent that?

ncoden · 2016-12-06T15:31:46Z

@oxyc True. I mistaken it with .gitignore

Mistaken `.gitignore` with `.npmignore`. The `/vendor` directory should be ignored by git, not npm.

ncoden · 2016-12-06T22:36:34Z

@hugogiraudel @kball Finally, I import normalize with ../_vendor/normalize-scss/sass/normalize. I agree a shorter path like ../_vendor/normalize-scss/normalize is more beautiful, but It is too complicated to make with the current gulp config file gulp/config.js.

ncoden · 2016-12-07T19:43:25Z

@hugogiraudel We still have issues with our Sass dependencies.

Because of Bower does not have its own registry like NPM, and uses the Github one, it does not find /vendor which is ignored by git.

The solutions we see for now is to:

Drop the Bower support: we can't (https://twitter.com/kbal11/status/805891962993713152)
Pick up the useful part of our dependencies into the Foundation code
Import /vendor in Github (but with only the required file of our dependencie moved to it)

What do you think ? Do you know how we could resolve this problem ?

KittyGiraudel · 2016-12-07T21:41:36Z

I think I would go with cherry-picked what you need from your dependencies while preserving link to original source in your documentation (SassDoc or whatever). That would make the building process easier and is likely to be enough for now.

ncoden · 2016-12-07T21:45:29Z

I just found a stupidly simple idea and submitted it to the rest of the team on Slack:

Why do not simply provide a 📦 dist/foundation.scss distribution file with all the dependencies included, instead of trying to make our scss/foundation.scss usable with an other build ?

@ncoden [22:35]
We want that the developers can import the Foundation SCSS into their project via Bower or NPM, and compile it with their own build process. But the problem(s) is with our Sass dependencies :

❌ We can’t include it as real dependencies ourselves (can’t assume the path to them).

❌ We can’t ask the developers to manually include it as real dependencies (too complicated, ).

❌ We can’t publish as dev dependencies to NPM and Bower without publishing it on GitHub (Bower uses the GitHub repository).

But… in fact we only want to provide a SCSS that can be used immediately : a distribution SCSS 📦. Why don’t we do like for the Js and CSS : publish a dist/foundation.scss file that also includes all our dev dependencies ?

It is more logical after all. scss/ is our sources. They should not be imported outside of Foundation to be compiled by an unknown build process. But distribution files are made for that.

Also, the dependencies can change a lot during the developement, the dist file is changed only on new releases.

@hugogiraudel What do you think ?

Edit: I checked, and it is what you do with SassyLists. :)

ncoden · 2016-12-07T21:49:23Z

See also: the current temporary PR for Bower: #9469

Prepare a better way to handle SCSS dependencies. Revert: - foundation#9430: add normalize dependency - foundation#9458: import Sass dependencies in vendor folder - foundation#9469: [TMP] Fix Sass dependencies import for Bower

ncoden added the scss label Dec 6, 2016

This was referenced Dec 6, 2016

Fix #9412 - Import normalize-scss with its full path #9449

Closed

Fix -zf-each-breakpoint without small #9441

Merged

[Build] Normalize should be a Bower Dependency in project and Templates #9412

Closed

KittyGiraudel reviewed Dec 6, 2016

View reviewed changes

Fix /_vendor ignore in .gitignore

1974abd

Mistaken `.gitignore` with `.npmignore`. The `/vendor` directory should be ignored by git, not npm.

michaelBenin mentioned this pull request Dec 6, 2016

Foundation normalize.scss path broken michaelBenin/react-ssr-spa#12

Closed

Move Sass dependencies from _vendor/scss to _vendor

f61a7fb

ncoden force-pushed the fix/9412-normalize-scss-import-2 branch from ba321a3 to f61a7fb Compare December 6, 2016 22:30

kball merged commit 088eb33 into foundation:develop Dec 6, 2016

ncoden mentioned this pull request Dec 9, 2016

Support includePath option theJian/gulp-shopify-sass#7

Closed

ncoden mentioned this pull request Dec 9, 2016

Add SCSS compilation into SCSS dist file #9478

Closed

ncoden mentioned this pull request Dec 21, 2016

[WIP] Better Sass dependencies management #9542

Closed

5 tasks

ncoden deleted the fix/9412-normalize-scss-import-2 branch April 7, 2018 17:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #9412 - import Sass dependencies in vendor folder #9458

Fix #9412 - import Sass dependencies in vendor folder #9458

ncoden commented Dec 6, 2016

KittyGiraudel Dec 6, 2016

ncoden Dec 6, 2016 •

edited

Loading

kball Dec 6, 2016

ncoden Dec 6, 2016

kball Dec 6, 2016

oxyc commented Dec 6, 2016 •

edited

Loading

ncoden commented Dec 6, 2016

oxyc commented Dec 6, 2016

ncoden commented Dec 6, 2016

ncoden commented Dec 6, 2016 •

edited

Loading

ncoden commented Dec 7, 2016 •

edited

Loading

KittyGiraudel commented Dec 7, 2016

ncoden commented Dec 7, 2016 •

edited

Loading

ncoden commented Dec 7, 2016 •

edited

Loading

Fix #9412 - import Sass dependencies in vendor folder #9458

Fix #9412 - import Sass dependencies in vendor folder #9458

Conversation

ncoden commented Dec 6, 2016

Changes:

KittyGiraudel Dec 6, 2016

Choose a reason for hiding this comment

ncoden Dec 6, 2016 • edited Loading

Choose a reason for hiding this comment

kball Dec 6, 2016

Choose a reason for hiding this comment

ncoden Dec 6, 2016

Choose a reason for hiding this comment

kball Dec 6, 2016

Choose a reason for hiding this comment

oxyc commented Dec 6, 2016 • edited Loading

ncoden commented Dec 6, 2016

oxyc commented Dec 6, 2016

ncoden commented Dec 6, 2016

ncoden commented Dec 6, 2016 • edited Loading

ncoden commented Dec 7, 2016 • edited Loading

KittyGiraudel commented Dec 7, 2016

ncoden commented Dec 7, 2016 • edited Loading

ncoden commented Dec 7, 2016 • edited Loading

ncoden Dec 6, 2016 •

edited

Loading

oxyc commented Dec 6, 2016 •

edited

Loading

ncoden commented Dec 6, 2016 •

edited

Loading

ncoden commented Dec 7, 2016 •

edited

Loading

ncoden commented Dec 7, 2016 •

edited

Loading

ncoden commented Dec 7, 2016 •

edited

Loading