Fix plugin-css, plugin-sass #150
Conversation
🦋 Changeset detectedLatest commit: cbda777 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Deploying with
|
| Latest commit: |
cbda777
|
| Status: | 🚫 Deploy failed. |
drwpow
force-pushed
the
fix/plugin-css
branch
from
a9123e7 to
ad2b14c
Compare
drwpow
force-pushed
the
fix/plugin-css
branch
from
ad2b14c to
cbda777
Compare
| @@ -96,6 +96,27 @@ describe('@cobalt-ui/plugin-css', () => { | |||
| expect(fs.readFileSync(new URL('./actual.css', cwd), 'utf8')).toMatchFileSnapshot(fileURLToPath(new URL('./want.css', cwd))); | |||
| }); | |||
|
|
|||
| test('transform', async () => { | |||
There was a problem hiding this comment.
plugin-css’ transform() API was missing a test; now it should catch regressions a little better.
|
🤔 Cloudflare failure seems to be a flake failure on their end ( |
| options?: { | ||
| prefix?: string; | ||
| suffix?: string; | ||
| // note: the following properties are optional to preserve backwards-compat without a breaking change |
There was a problem hiding this comment.
Does this mean they will be removed with the const token = options?.tokens?.find((t) => t.id === refID); find and warn bellow once there is a breaking change?
There was a problem hiding this comment.
We’re following semver in this package, so there shouldn’t be any breaking changes until 2.0.0.
This was just an unintentional internal change that accidentally led to a breaking change. It was triggered by mismatching versions of plugin-css (+/- 1.6.0) and plugin-sass (+/- 1.3.2). This change just allows for (hopefully) graceful fallback between the two plugins. The optional chaining method just allows an older version of the plugin to omit that data, and it will, still render a result.
There was a problem hiding this comment.
Hidden cross-dependencies like this are an antipattern in general, and something I should clean up better. Just at the moment, it felt like saving work / reducing bugs to have plugin-sass rely on plugin-css, but over time they’ve become too tightly-coupled and I think there needs to be a common core they share that allows for safer version drift between the two (irrelevant for consumers; this is just an implementation detail I’m musing on).
| const variableId = refID + normalizedSuffix; | ||
|
|
||
| return `var(${generateName(variableId, token)})`; | ||
| return `var(${options?.generateName?.(variableId, token) ?? defaultNameGenerator(variableId, options?.prefix)})`; |
There was a problem hiding this comment.
So does this mean varRef is a part of the plugin's public API? Because otherwise this is unnecessary. Both parts of this logic...
- Was a custom name generator passed? If not use the default one
- Was a prefix passed? If so, pass it to the default generator
...are already accounted for in the generateName argument (see line 76) by way of currying.
There was a problem hiding this comment.
So does this mean varRef is a part of the plugin's public API?
Maybe not “public”, but used in the Sass plugin. And so mismatched versions would throw an error due to the changed signature. It’s a TODO item for cleanup
Changes
Fixes accidental regression in
transform()APIs and plugin-sass ↔ plugin-css interop (mentioned in #145).How to Review