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

Improve heft-webpack-basic-tutorial to illustrate .scss support #3

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

octogonz
Copy link
Collaborator

@octogonz octogonz commented Jun 17, 2021

  • Upgrade to Webpack 5
  • Enable webpack.config.js options for .scss support
  • Improve webpack.config.js docs
  • Add examples of .scss usage

Over time I'm hoping for this project to accumulate a reasonable set of "best practices" for a real world web application. Although it's a "tutorial", I'd like to to aim for this sample to be:

  • full featured
  • well documented
  • realistic

That seems more useful to people than trying to make it simplistic.

These same settings will be integrated into the @rushstack/heft-web-rig setup. Then we'll add a heft-webpack-rig-tutorial, and heft-webpack-basic-tutorial will be the same thing but with all the rig settings inlined.

CC @iclanton @halfnibble

@octogonz
Copy link
Collaborator Author

BTW I noticed that HBO has been using:

Let me know if you have opinions about that approach.

@octogonz
Copy link
Collaborator Author

I also noticed that HBO uses *.module.css to opt-in to modular CSS. Whereas Heft's example projects simply use modular CSS everywhere. This article suggests modular by default, but with *.global.css to opt out of modular CSS.

@halfnibble
Copy link

I also noticed that HBO uses *.module.css to opt-in to modular CSS. Whereas Heft's example projects simply use modular CSS everywhere. This article suggests modular by default, but with *.global.css to opt out of modular CSS.

I like the idea in that article. I think we'll need to make this configurable though. Even within our monorepo, we have some projects with different rules for .scss modules. So the current Heft implementation is not ideal. If we agree it should be configurable, then question the becomes, what do we make the default?

@halfnibble
Copy link

BTW I noticed that HBO has been using:

Let me know if you have opinions about that approach.

It looks like Webpack's docs recommend using min-css-extract-plugin in production mode to break up assets into more async loads. And they appear to recommend style-loader for local development only.

Interesting. It looks like this was meant to be a piece of code, but instead manifested as a break or paragraph.

Screen Shot 2021-06-16 at 11 54 20 PM

@@ -3,7 +3,7 @@

"compilerOptions": {
"outDir": "lib",
"rootDir": "src",
"rootDirs": ["src/", "temp/sass-ts/"],

Choose a reason for hiding this comment

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

Maybe @iclanton will remember for certain, but I vaguely remember there being something counterintuitive about the "rootDirs" field that required us to keep both "rootDir" and "rootDirs". I think "rootDir" is used for calculating the root path and if you don't have a TS file in your "src" folder, then you might lose your folder structure without it being also specified.

Copy link
Collaborator Author

@octogonz octogonz Jun 19, 2021

Choose a reason for hiding this comment

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

@halfnibble good catch!

You are right, despite the name, rootDir and rootDirs are very different settings:

  • rootDir explicitly specifies the folder that will be used as the basis for lib output; otherwise it will be inferred from the common parent of the files, include, etc
  • rootDirs specifies directories with additional typings that will be overlayed on the main src directory tree, under the assumption that those .js files will all get copied into the same output folder.

Specifying rootDirs does not influence the rootDir.

As an experiment, with "rootDir": "src" deleted in this branch, if I move all my files to be src/x/*.ts they wrongly get compiled to lib/*.ts instead of lib/x/*.ts.

The naming of these settings is very misleading.

@octogonz
Copy link
Collaborator Author

octogonz commented Jun 17, 2021

I like the idea in that article. I think we'll need to make this configurable though. Even within our monorepo, we have some projects with different rules for .scss modules. So the current Heft implementation is not ideal. If we agree it should be configurable, then question the becomes, what do we make the default?

From what I've read, CSS modules isn't just a technical feature, it is a policy agenda to discourage global CSS. The reason is to avoid a situation where somebody adds global CSS to a library and it "works fine" for their app, but it breaks someone else's app because of a global naming conflict. This I think motivates a standard where CSS is local-by-default, and then you can opt out by using the filename.global.scss or filename.global.css. Or by specifying the :global selector in your file.

In a monorepo it's pretty important to have consistent conventions, to avoid accidents where someone adds filename.scss to someone else's project, and then is surprised to find that the defaults were different.

CC @bartvandenende-wm

@KevinTCoughlin
Copy link
Member

I like the idea in that article. I think we'll need to make this configurable though. Even within our monorepo, we have some projects with different rules for .scss modules. So the current Heft implementation is not ideal. If we agree it should be configurable, then question the becomes, what do we make the default?

From what I've read, CSS modules isn't just a technical feature, it is a policy agenda to discourage global CSS. The reason is to avoid a situation where somebody adds global CSS to a library and it "works fine" for their app, but it breaks someone else's app because of a global naming conflict. This I think motivates a standard where CSS is local-by-default, and then you can opt out by using the filename.global.scss or filename.global.css. Or by specifying the :global selector in your file.

In a monorepo it's pretty important to have consistent conventions, to avoid accidents where someone adds filename.scss to someone else's project, and then is surprised to find that the defaults were different.

CC @bartvandenende-wm

👍 In addition to encapsulation, CSS modules, once supported by browser engines, will provide performance benefits -
https://github.com/WICG/webcomponents/blob/gh-pages/proposals/css-modules-v1-explainer.md.

@octogonz
Copy link
Collaborator Author

@KevinTCoughlin also mentioned that autoprefixer can produce unhelpful output if it doesn't have an appropriate browserlist config.

@octogonz
Copy link
Collaborator Author

@Claudiazhaoya said that this is coming eventually microsoft/rushstack#2732

{
// Compiles SASS syntax into CSS
// https://www.npmjs.com/package/sass-loader
loader: 'sass-loader',
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@bartvandenende-wm pointed out that this recipe will apply SASS transformations to .css files, which we don't want

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We're thinking to change the rig to work like this:

  • .css : NO sass, NO autoprefixer, NO modules
  • .scss : sass, autoprefixer, modules
  • .global.scss : sass, autoprefixer, NO modules

...and if someone wants CSS with modules, they can simply use the .sass file extension, since SASS is basically a superset of CSS.

There's a small performance penalty for applying SASS to a plain CSS file, but @halfnibble pointed out that the extra syntax validation probably justifies that cost.

@rootical
Copy link

Is there any chance for dart-sass support still? The PR seems to be forgotten

@octogonz
Copy link
Collaborator Author

octogonz commented Feb 4, 2022

Is there any chance for dart-sass support still?

@rootical thanks for bringing this up. I assumed that heft-sass-plugin was already using dart-sass, but apparently it is still using node-sass.

The PR seems to be forgotten

This particular PR #3 did get forgotten -- we implemented the ideas in our company's internal rig and never got around to finishing up the public code sample.

However it came up again, because the Rush Stack websites are migrating from Ruby->TypeScript, which has introduced several Webpack projects that need to be maintained as open source. I'm encountering issues with their CSS, so I'm going to see if I can revamp @rushstack/heft-web-rig with an up-to-date and full-featured configuration. I'll see if I can fix the node-sass issue at the same time. And then we can sync all that into heft-webpack-basic-tutorial and finally get this PR merged, too.

Update: microsoft/rushstack#3204

…to octogonz/scss-improvements

# Conflicts:
#	common/config/rush/pnpm-lock.yaml
#	common/config/rush/repo-state.json
#	heft/heft-node-basic-tutorial/package.json
#	heft/heft-node-jest-tutorial/package.json
#	heft/heft-node-rig-tutorial/package.json
#	heft/heft-webpack-basic-tutorial/package.json
#	other/packlets-tutorial/package.json
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.

4 participants