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

npm link doesn't play nicely with Sass imports #715

Closed
shawnbot opened this issue Mar 8, 2019 · 1 comment
Closed

npm link doesn't play nicely with Sass imports #715

shawnbot opened this issue Mar 8, 2019 · 1 comment
Assignees
Labels
area: tooling 💓collab a vibrant hub of collaboration ⭐️rep an industry leading reputation type: bug 🐞

Comments

@shawnbot
Copy link
Contributor

shawnbot commented Mar 8, 2019

It's currently not possible to npm link this repo into a project that imports our Sass files without adding src/ after @primer/css in the import path. For instance:

@import "@primer/css/support/index.scss";

This works when you npm install @primer/css because we copy src/* up a level in script/prepublish. But if you npm link path/to/primer/css, that script doesn't run, support/index.scss doesn't exist because it lives in src/, and you have to change it your import path:

@import "@primer/css/src/support/index.scss";

This is not great. We have options, though!

  1. Add node_modules/@primer/css/src to Sass import paths

    This is the simplest option and involves only updating our docs, but it's also kind of an ugly hack.

  2. Move src/* up to / in git

    This fixes the issue, but clutters the repo root with lots of directories, some of which contain SCSS files while others (pages, script, bin, lib, etc.) don't. I prefer to silo off the style sources from other types of files, but I'm open to it if this doesn't bother anyone else.

  3. Symlink src/* to / on installation

    We might be able to detect one npm's automatic environment variables to determine if we're running "locally" and add those files to .gitignore so that they're not committed accidentally. I tested npm link on my machine and it doesn't look like we can hook into it with a link script in package.json. But install runs automatically as part of linking, so we might be able to do it there.

    ⚠️ If we're going to do this, I really want us to be diligent about not relying on those files existing in development. We might want to remove the symlinks before tests are run to guard against false positives when resolving Sass import paths, for instance.

  4. Sass custom importer sorcery?

    Nope nope nopity nope. Don't even think about it.

@shawnbot shawnbot added bug area: tooling ⭐️rep an industry leading reputation 💓collab a vibrant hub of collaboration labels Mar 8, 2019
@shawnbot shawnbot self-assigned this Mar 8, 2019
@shawnbot
Copy link
Contributor Author

Another potential gotcha that I just ran into in github/github, and which is pretty specific to that app, is that if you run npm link with a different version of Node than you've last run npm install in this repo, it'll fail because the node-sass native binaries (which are Node version-specific) won't match. (npm runs the prepare script automatically as part of install).

The simplest fix is just to rm -rf node_modules in this repo first. The more "correct" solution is probably not to run dist in our prepare script, as in #718.

@simurai simurai mentioned this issue Mar 12, 2019
10 tasks
@yaili yaili added type: bug 🐞 and removed bug labels Feb 27, 2020
@jonrohan jonrohan removed the v12 label Apr 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: tooling 💓collab a vibrant hub of collaboration ⭐️rep an industry leading reputation type: bug 🐞
Projects
None yet
Development

No branches or pull requests

3 participants