Path to sass-mq is dependent on specific node_modules location

[penx]

This line is causing us an issue:

govuk-frontend/dist/components/globals/helpers/_media-queries.scss

Line 14 in ad2022e

@import "./node_modules/sass-mq/mq";

This assumes that sass-mq is installed in a node_modules folder relative to the root folder used by the Sass build system.

This isn't always the case, e.g.

1. If Sass is configured to resolve links relative to file location rather than parent project root
2. If the parent project uses a different version of sass-mq to govuk-frontend, npm won't flatten this dependency and the version needed by govuk-frontend actually lives in ./node_modules/govuk-frontend/node_modules/sass-mq
govuk-frontend
3. if used in a monorepo (lerna, yarn workspaces) then node_modules are hoisted in to a parent directory.

I suspect this can be fixed by swapping it with @import "sass-mq/mq", which will leave node to deal with the module resolution.

[NickColley]

Thanks for raising! Do you have an example of the build where this fails for us to reproduce? It may help us fix this quicker. 😄

[penx]

@marksy was having this issue, I think on this branch, though perhaps he can confirm

https://github.com/penx/govuk-react/tree/feature/sassExtract

[penx]

failing that I could try create a simple example if that would help?

[penx]

@NickColley @igloosi I have created a failing example with instructions here https://github.com/penx/govuk-monorepo-example

[penx]

Note that Webpack sass-loader provides a mechanism to load dependencies from node_modules:

@import "~sass-mq/mq";

However this doesn't seem to be a standard Sass convention, but personally I think it is preferred to importing from an assumed relative path to node_modules.

[NickColley]

@penx thanks so much for the failing example! 🙌

[kr8n3r]

the proposed change in #637 should fix that
https://github.com/alphagov/govuk-frontend/pull/637/files#diff-b47fe464b7770f8ba6b5ce3bf61eceb9R14

[kr8n3r]

the other options would be to expose that path in config options
@penx or @marksy can you forsee any issues with that when you include it in your project?

[penx]

@igloosi that may work, but would require the parent project to know where the package was hoisted to (which could potentially vary) rather than defer this resolution to node.

Any reason for this instead of Eyeglass?

[penx]

failing that we could have the govuk-frontend Sass => JS variables conversion go in to its own project that wasn't a monorepo and do the conversion as part of its build, then have govuk-react depend on this interim package

[NickColley]

@penx (while we try and find other solutions) is it possible to avoid hoisting node_modules? Or avoiding yarn?

[penx]

@NickColley using a monorepo is pretty core to the current architecture of govuk-react, and I am pretty certain other monorepo approaches (lerna + npm) would have the same issue, but if we move the conversion to another repo then this may work, unless I'm missing something @marksy?

[NickColley]

I think I'm maybe misunderstanding but is hoisting node_modules required for a monorepo approach?

Looks to have more context on hoisting: https://yarnpkg.com/blog/2018/02/15/nohoist/

[penx]

Good point, I thought it was, which led me to find this:

https://yarnpkg.com/blog/2018/02/15/nohoist/

So we could probably use that.

Related, I thought lerna hoisted by default but apparently it doesn't

https://github.com/lerna/lerna/blob/master/doc/hoist.md

Also in the above link

some tooling does not follow the module resolution spec closely, and instead assumes or requires that dependencies are present specificially in the local node_modules directory. To work around this, it is possible to symlink packages from their hoisted top-level location, to individual package node_modules directory. Lerna does not yet do this automatically, and it is recommended instead to work with tool maintainers to migrate to more compatible patterns

I think it would still be good to have hoisting support in the medium term though could see why this may be low priority 😀

[penx]

Interestingly I seem to have the same issue when not in a monorepo!

https://github.com/penx/govuk-frontend-js

git clone [email protected]:penx/govuk-frontend-js.git
cd govuk-frontend-js
node build.js

Error: Can not determine imported file for url './node_modules/sass-mq/mq.scss' imported in govuk-frontend-js/node_modules/@govuk-frontend/globals/helpers/_media-queries.scss

[kr8n3r]

@penx this will be fixed in the next release of Frontend

[kr8n3r]

relased in https://github.com/alphagov/govuk-frontend/releases/tag/v0.0.29-alpha