Repository navigation
Add support for CSS preprocessors - #5
Conversation
`sass-content-mapper` doesn’t expose types anyway. So we can safely import it from a dev dependency.
remcohaszing
left a comment
There was a problem hiding this comment.
I've added in a few strictness TypeScript strict checks - noUncheckedIndexedAccess, exactOptionalPropertyTypes and noImplicitOverride are on
I accidentally removed them while refactoring. We can discuss more TypeScript options in a follow-up. Let’s keep this focused for now.
The tests harnesses are very similar. Should we extract this out now, and if so, should we ship it from the core package
I extracted some things and made abstractions in content-mapper-test-utils. I did have some issues integrating them here though, because of the specific proc args you used. Please let me know if you can use that, and if there’s room for improvement.
The protocol's
watchedFilesis set per project, so changes to partials don't refresh types in editors. Should we raise per-transform dependencies with the TypeScript team?
TBH I think this feature is stretching the scope of content mappers. You can raise the issue, but I wouldn’t be surprised if it gets closed.
If we really want to do this correctly, we could process Sass imports and convert them to TypeScript imports in the virtual files. Then we could even map class names to external files. This is a way more advanced feature though, as it requires more knowledge of parsing and resolving Sass.
Are you OK if we add tests using
tsc --lspin a follow-up?
Yep! But also let’s see how much that adds first. We don’t use the most advanced LSP features here, only definitions.
Did you want tests covering the minimum supported version of Sass? If so, any thoughts on the best way to approach that?
I haven’t thought of that. For now I think it’s fine to only test one version. Maybe stick with the oldest one supported?
When ready, how would you like to merge this? The first commit is the restructuring, to preserve history.
I already merged some changes into main. I think the PR is currently in pretty good shape when it comes to focus. So let’s merge this when ready. The PR does need a changeset now. 😉
| node-version-file: package.json | ||
| - run: npm ci | ||
| - run: npx tsc | ||
| - run: npm --workspaces exec -- tsc --build |
There was a problem hiding this comment.
I reverted this specific line.
If you use npm --workspaces run / npm --workspaces exec, the command is run for each workspace. This has a couple of downsides.
- A command is spawned for every workspace.
- The CWD is set to the workspace, which often affects logging of relative paths.
A lot of tools support a monorepo setup, including TypeScript. TypeScript supports this using project references, which is configured in the root tsconfig.json.
If you configure tools like that, you typically get:
- Better performance (TypeScript 7 can even parallelize builds now)
- Better third party tooling integration (editor integration for some tooling picks up a config file in the repo root)
- Correct relative paths (which VSCode and my terminal emulator turn into links)
There was a problem hiding this comment.
Ah, I think I missed that you'd added project references. I've used them before and agree that they're a great way to keep monorepos fast - thanks for picking that up!
This is a draft implementation for early feedback.
Notes and decisions:
sassandsass-embeddedare supported, but the latter is preferred for performance reasons. Vite also recommends the embedded package.sassand maps classes back through source maps, rather than parsing statically. This allows us to handle nesting,&-suffix,#{}, loops and mixins the same way that a build does.originalPositionFor.Syntaxobject.noUncheckedIndexedAccess,exactOptionalPropertyTypesandnoImplicitOverrideare on - but happy to remove those if you're not a fan.engines, and@types/nodeis set to 22 so that we don't accidentally use a newer API. CI tests Node 24 and 26.Open questions:
./testing)?watchedFilesis set per project, so changes to partials don't refresh types in editors. Should we raise per-transform dependencies with the TypeScript team?tsc --lspin a follow-up?