csstype icon indicating copy to clipboard operation
csstype copied to clipboard

v3.0.10 contains breaking changes from v3.0.9

Open dfernandez79 opened this issue 4 years ago • 10 comments

The version v3.0.10 added a string & {} to types that didn't supported string making it a breaking change from v3.0.9:

import type { Property } from 'csstype';

// Fails in 3.0.9 but works in 3.0.10
const fontWeight: Property.FontWeight = 'anything'; // ok 'anything' is a string & {}
console.log(fontWeight)

This difference makes TS fail when NPM resolves to different (but semver compatible) versions of csstype:

import styled from 'styled-components'; // npm resolves to 3.0.9
import otherLibProps from 'other'; // npm resolves to 3.0.10

const Component = styled.div`
   /* fails string & {} is not assignable to fontWeight in 3.0.9 */
   ${otherLibProps}
`

While the previous situation is a red flag about NPM hoisting issues, it's a possible situation that I've seen in repos with dependencies that internally use older csstype versions (like material-ui v4).

Possible solutions:

  • The addition of string & {} to fontWeight was an error, because it makes the type more permissive and not assignable to type declarations of previous versions. Removing string & {} in that case should be harmless as it doesn't affect the TS LSP auto-complete. This will not fix #125 in 3.x.
  • Keep the string & {} but mark the version as v4.0.0: it will not solve the TS compilation issues, but at least the incompatibility is explicit and libraries using ^3. in the semver range will not catch the issue by surprise. This fixes #125 but in a 4.x version.

dfernandez79 avatar Dec 13 '21 14:12 dfernandez79

I'm experiencing the same issue. JSS used by Material UI 4.x depends on csstype 3.x while Material UI itself depends on 2.x. Until now the types from 3.x and 2.x were compatible with each other but this change let my builds fail.

levrik avatar Dec 29 '21 14:12 levrik

Where do you removestring & {} from to fix? I am dealing with this issue here https://stackoverflow.com/questions/70714921/what-typescript-version-to-use/70715922#70715922

marianatandon avatar Jan 18 '22 19:01 marianatandon

I don't consider this a breaking change in a typing perspective. Then basically all changes should be considered breaking.

The type safety issue should be solved with literal template types later on. But the compatibility issue with older versions of CSSType by using e.g. Material UI is nothing we can account for. It should be upgraded on their end instead of restricting features on our end.

frenic avatar Jan 20 '22 10:01 frenic

... Then basically all changes should be considered breaking.

Not all the type changes are breaking: anything that maintains assignment compatibility doesn't make the type-checking fail (adding new optional properties, adding optional generic types, making function arguments general).

But before going into the rabbit hole of the discussion of the breaking changes.

The problem described in the issue not only happens with Material v4 which uses csstype v2.6 (in that case I think the breaking change is expected). It also happens when you have a mix of csstype 3 < 3.0.10 and 3.0.10.

In theory, NPM should dedupe csstype in those situations. But, in practice, I found that issue in 2 of 3 projects that I work on at my company. I was able to solve it by downgrading to 3.0.9 (fortunately it was a package that I control), and by re-creating the package-lock.json (so NPM properly dedupes the csstype dependency). With that workaround, I got the type-check of styled-components working again, and for the project using MUI v4, I had to pin the dependency to 3.0.9.

The unexpected thing was that CI/CD started to fail after a transitive dependency update that didn't change to any major version.

I understand that reverting may be the worst. But, I wonder how I can help to prevent this situation in the future. (I consider the change in 3.0.10 very unfortunate 😞 , as I don't care about the feature introduced by #125 and the type widening introduced by adding string to the union brings other headaches like #149).

Now back to the discussion of the breaking changes. How may we define an automated test to verify if a version is breaking? Perhaps is possible to add tests that checks some cases against the previous minor version? (BTW nice work in those tests using the TSC API). Does it make sense?

dfernandez79 avatar Jan 20 '22 12:01 dfernandez79

I was able to solve it by downgrading to 3.0.9 (fortunately it was a package that I control), and by re-creating the package-lock.json (so NPM properly dedupes the csstype dependency). With that workaround, I got the type-check of styled-components working again, and for the project using MUI v4, I had to pin the dependency to 3.0.9.

How did you downgrade (pin the dependencies to 3.0.9) and recreate the file? I am trying to solve this on my own as well.

marianatandon avatar Jan 20 '22 19:01 marianatandon

@marianatandon You'll need to make NPM to resolve 3.0.9.

The good news is that 3.0.10 is relatively new. So, the chances are that any transitive dependencies require 3.0.x instead of 3.0.10.

If you use NPM v8 (or PNPM) you can force the version with the overrides configuration: https://docs.npmjs.com/cli/v8/configuring-npm/package-json#overrides

If you use Yarn you have the resolutions option: https://classic.yarnpkg.com/lang/en/docs/selective-version-resolutions/

If you use an older version of NPM is not that easy:

  • You can try this package: https://www.npmjs.com/package/npm-force-resolutions
  • You can try installing [email protected] directly. When NPM does the de-duplication of dependencies if they are compatible it will use the version that you installed in the parent repo (this is kind of frustrating because many times NPM resolves transitive deps to 3.0.10 and duplicates it 🤷‍♂️ ).
  • You can manually edit your package-lock.json and change the resolution from 3.0.10 to 3.0.9

dfernandez-asapp avatar Jan 20 '22 20:01 dfernandez-asapp

This doesn't seem like a breaking change to me either. You're getting an error because of module resolution and conflicting versions, but there was no type narrowing so theoretically no previous contract was broken, hence not a breaking change. One could argue that this should have been a minor release instead of patch, but your situation would have been no different I guess. Well, SemVer wasn't really designed for this so this is kind of pointless anyway. I agree that in practice it might be wise to release 3.0.11 with a revert and 4.0.0 just to avoid too much trouble downstream (depending on the community response).

villasv avatar Feb 16 '22 21:02 villasv

@villasv I agree with you about SemVer. The definition of "breaking change" is vague, and the use of typings wasn't even a use case of Node dependency handling.

However, type widening may break a previous contract:

type OldFontWeight = 'bold';
type NewFontweight = 'bold' | (string & {})

const oldWeight: OldFontWeight = 'bold'
const weight:NewFontweight = oldWeight; // ok

function doSomething(w:OldFontWeight) {}

/*
Argument of type 'NewFontweight' is not assignable to parameter of type '"bold"'
*/
doSomething(weight);

This scenario may happen if:

  • You mix a library using 3.0.9 with another using 3.0.10 (the dependencies are pinned or not hoisted for other reasons).
  • or you have a function that doesn't use csstype but is compatible for a specific sub-set of values. (this scenario doesn't involve dependency handling).

To me is about expectations: when you upgrade to a patch version, a CI compilation failure is unexpected.

My situation was exactly like that: I maintain a design system that uses csstype. I did an upgrade from 3.0.9 to 3.0.10. Patch upgrade. Everything is good. All tests passing.

Suddenly, I received notifications from other teams using the library: the CI compile step failed. The reason: their project use styled-components and consume values from the design system. A tagged literal is a function call:

import styled from 'styled-components';
import {style} from 'the-design-system';

// Type error: the styled-components tag literal function uses 3.0.9 (narrow type)
// style uses 3.0.10 (wide type)
const Comp = styled.div`
  ${style}
`

And unless you declare csstype as a peer-dependency, there is no guarantee that the package manager will not duplicate versions.

In a situation like this, v4 in the version number is a good indicator of what to expect. As a project maintainer, I know it's hard to identify breaking changes. I wonder if we can reify the notion of "breaking change" with unit tests.

dfernandez79 avatar Feb 17 '22 19:02 dfernandez79

About testing for breaking changes, perhaps we can do something like this: https://github.com/frenic/csstype/blob/master/tests/fixtures/typecheck.ts#L148

But, with the previous minor version:

import * as PreviousMinor 'previousminor-csstype';
const differentMinorVersions: CSS.Properties = {} as PreviousMinor.Properties

function usesPreviousMinor(props: PreviousMinor.Properties) {}
usesPreviousMinor(differentMinorVersions)

This will check both cases: assignment and function call. The version of previousminor-csstype can be updated with a script, by modifying: https://github.com/frenic/csstype/blob/master/tests/fixtures/package.json

dfernandez79 avatar Feb 17 '22 19:02 dfernandez79

@dfernandez79 thank you! I was getting close to figuring out what was going on but not sure I would have got there if I didn't find this issue report.

finally resolved after a long day of head-scratching with resolutions for yarn 3 added to main package.lock.

  "resolutions": {
    "csstype@^3.0.2": "3.0.9"
  }

I was just trying to move a bunch of [mui@4] React components from an old working repo to a new monorepo. Even copying all deps and versions from the old repo didn't avoid this issue of course and it was very unclear as to where the problem lay so I spent a lot of time looking at all the wrong things.

lecstor avatar Feb 18 '22 06:02 lecstor