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

Proof of concept: migrate to dart-sass #390

Closed
wants to merge 1 commit into from
Closed

Conversation

oscarduignan
Copy link
Contributor

The key bit of this proof of concept is that by bodging the includePaths we've made it so that we get the behaviour we had before from libsass which means we don't need to change the relative imports we have in our sass paths that will require sass load paths be updated by consumers. Where some of our consumers are using different ways of loading packages and they won't always be in node_modules folder, and some don't have the option of updating their load paths (govuk prototype kit) users.

The key bit of this proof of concept is that by bodging the includePaths
we've made it so that we get the behaviour we had before from libsass
which means we don't need to change the relative imports we have in our
sass paths that will require sass load paths be updated by consumers.
Where some of our consumers are using different ways of loading packages
and they won't always be in node_modules folder, and some don't have the
option of updating their load paths (govuk prototype kit) users.
@@ -100,6 +100,7 @@
"rollup-plugin-babel": "^4.4.0",
"rollup-plugin-commonjs": "^10.1.0",
"rollup-plugin-node-resolve": "^5.2.0",
"sass": "^1.77.8",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would also need to remove node-sass, but didn't need to for the proof of concept

@@ -22,7 +22,7 @@ gulp.task('scss:compile-all-govuk-and-hmrc', () => gulp.src(`${configPaths.src}a
.pipe(plumber(errorHandler))
.pipe(sourcemaps.init())
.pipe(sass({
includePaths: ['node_modules'],
includePaths: ['node_modules', 'node_modules/govuk-frontend/dist', 'node_modules/govuk-frontend/dist/govuk/components'],
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we have two types of relative imports at the moment ../../govuk-frontend and ../../../../govuk-frontend which - I think govuk-frontend gets around issues with load paths by not importing any css from node_modules in their css - they just have their own css / stuff vendored in that's a single file

the way this works, we don't actually want to use any css from either of those new paths, we just want to be able to relatively traverse to places in govuk-frontend from hmrc-frontend css when we're compiling it when the hmrc-frontend css is not inside node_modules - all our consumers compile the hmrc css in the context of hmrc-frontend being inside a folder (like node_modules or lib in the case of webjars) where govuk-frontend will be also in that folder, so relative traversals for them work

@oscarduignan
Copy link
Contributor Author

superseded by #395 I'm closing this because future collaboration can happen in the other PR

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.

1 participant