feat(stylus): add initial stylus rule #908
Conversation
|
I live-coded this, here's a recording: I'll post it more widely too |
|
Also add |
packages/stylus/index.bzl
Outdated
| outputs = [css_output, map_output], | ||
| inputs = [src] + ctx.files.deps, | ||
| executable = ctx.executable._compiler, | ||
| arguments = [ |
There was a problem hiding this comment.
Probably we should enable compress and resolve-url.
Also, we should probably a way to pass the include
More info: http://stylus-lang.com/docs/executable.html
There was a problem hiding this comment.
adding a test for resolve-url shows an interesting consequence, TAL. Same problem as sourcemaps...
How would users want to use include? seems like that should just come from the path of the deps?
There was a problem hiding this comment.
I was thinking more something similar to the sass_binary and how they handle Sass include paths.
They have an additional attribute for this, I also think that with this approach it might be might be more performant since imports resolutions will be limited to the fine grained include paths installed of the entire deps.
https://github.com/bazelbuild/rules_sass#implicit-output-targets
In future also, we might want to make compress and sourcemap configurable by the user.
There was a problem hiding this comment.
Asked Jeremy, he says this attribute only exists on sass_binary for legacy reasons and it would be better not to include it for new code.
Added user-config for compress and sourcemap.
There was a problem hiding this comment.
What is the new alternative/suggested way without include paths?
As if users want to import let's say bootstrap in their scss file. They will need to do a full import without include paths?
@import '../../node_modules/bootstrap/scss/functions'Some context angular/angular#31619 (comment)
There was a problem hiding this comment.
I would expect that to be written @import 'bootstrap/scss/functions' - meaning the node_modules should always be in the sass include path. I didn't check if that actually works though.
There was a problem hiding this comment.
Afaik preprocess include paths are empty by default.
There was a problem hiding this comment.
yeah so I think we should add a test for this, and hard-code correct paths. I'm not sure users should have control over something that affects paths
There was a problem hiding this comment.
Yeah some tests here are mandatory. I mean we can start with hard coded paths and evaluate the option to add paths publicly if needs be. It’s easier to add than remove.
| console.error('Expected the css file to be transformed'); | ||
| process.exitCode = 1; | ||
| } | ||
| if (content.match(/\r|\n/)) { |
There was a problem hiding this comment.
Small nit: .test should be faster since you want a boolean check
packages/stylus/docs/install.md
Outdated
| Add the `@bazel/stylus` npm package to your `devDependencies` in `package.json`. | ||
|
|
||
| Your `WORKSPACE` should declare a `yarn_install` or `npm_install` rule named `npm`. | ||
| It should then install the rules found in the npm packages using the `install_bazel_dependencies' function. |
There was a problem hiding this comment.
There an apostrophe instead of a backtick after install_bazel_dependencies
|
rebased on new layout scheme for packages, PTAL |
Add compress and sourcemap as user-controlled options Address some code review comments
packages/stylus/src/BUILD.bazel
Outdated
|
|
||
| bzl_library( | ||
| name = "bzl", | ||
| testonly = True, |
There was a problem hiding this comment.
nit: not necessary unless depending on rules_webtesting:web bzl_library target
packages/stylus/src/package.json
Outdated
| "rootPath": "." | ||
| } | ||
| }, | ||
| "scripts": { |
There was a problem hiding this comment.
"scripts" no longer necessary in this package.json after packages refactor
This ensures we'll load the version of stylus we requested, not something else the user installed
Thanks for suggesting this in angular/angular#19058 @glebmachine