Update SassMQ import path

[kr8n3r]

In certain use cases as outlined in #636 the way we were importing SassMQ wasn't resolving correctly.
This change should fix that.

• Updated path in src/globals/helpers/_media-queries.scss
• includePaths: 'node_modules' added to gulp compilation task
• includePaths: 'node_modules' added to node-sass configuration in tests
• Changelog updated

[NickColley]

Does this require any documentation updates, or is this a transparent change for users?

Is there anyway we can have a failing test for this?

[penx]

the webpack sass-loader way of doing this would be "~sass-mq/mq" https://github.com/webpack-contrib/sass-loader#imports

[kr8n3r]

yes

[penx]

Is there any reason not to use the ~ syntax here? I think the current implementation would mean that the parent project would need to manually ensure sass-mq was on the resolution path for sass files, wheras ~ should always work as long as the parent project used sass loader.

Or are you saying that compatibility with Sass build systems that don't support ~ is more important than manual setup?

Would be good to see how existing Sass libraries distributed via npm that have Sass sub-dependencies handle this.

[36degrees]

Do you know what the support is for that syntax? We can make very few assumptions about the technologies involved our consumer's build pipelines, so unless that syntax is very well supported I would be wary of relying on it.

[penx]

I think the problem (as described at https://github.com/webpack-contrib/sass-loader#imports) is Sass doesn't have a default syntax for library imports.

The 3 options are:

1. ~
2. node_modules or ./node_modules
3. mixing relative + lib paths (@import "sass-mq/mq";)

I think all 3 infer either an assumption or requirement from the parent project.

I'll take a look around and see if I can find examples in other Sass projects dealing with this.

[penx]

related, but not found an answer yet:

sass/node-sass#1596
webpack-contrib/sass-loader#466

[kr8n3r]

~ is webpack specific, node-sass doesnt supoort that

[penx]

I made a couple comments on sass/node-sass#1596 and also found and commented on sass/sass#2350

Is it ok to assume the parent project will use node-sass (given that it's distributed via npm)?

Looks like eyeglass may be the recommended solution if so.

[NickColley]

Added an example based on the reproduction @penx gave us. I'm not sure this PR is enough to fix it...

Edit: Branch I'm talking about: https://github.com/alphagov/govuk-frontend/tree/fix-sass-mq-import-example-tests/examples/using-yarn-workspaces

[kr8n3r]

Does this require any documentation updates, or is this a transparent change for users?

with this option: no change
but if they use this option @import "node_modules/@govuk-frontend/all/all";
then it possibly won't work

Is there anyway we can have a failing test for this?
compile-individual-components test fails if you just change
@import "./node_modules/sass-mq/mq"; to @import "sass-mq/mq"; without adding node_modules to its config

[36degrees]

In #615 we made adding node_modules being in the consumers' Sass paths optional in the documentation, by making the 'default' import relative to the project root. Unless I'm misunderstanding, this looks like it would rely on consumers having node_modules back in their Sass path definition again.

If this is the only way to do it that shouldn't be a blocker, but we'll need to make sure we update the documentation appropriately.

[kr8n3r]

yeah if they choose the option without needing to specify the path definition then it feels wrong in having to ask them to do it regardless for this to resolve.

• we can revert back to @import "./node_modules/sass-mq/mq"; but wrap it in a variable so it can be modified for cases mentioned in the issues. Then there is no change for most users

Radical
Add Eyeglass to here as devDependency

More radical
go back to our own breakpoint mixins

[kr8n3r]

won't fix, closing in favour of #657