frenic / csstype

Strict TypeScript and Flow types for style based on MDN data
MIT License
1.7k stars 69 forks source link

v3.0.10 contains breaking changes from v3.0.9 #148

Open dfernandez79 opened 2 years ago

dfernandez79 commented 2 years ago

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:

levrik commented 2 years ago

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.

marianatandon commented 2 years ago

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

frenic commented 2 years ago

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.

dfernandez79 commented 2 years ago

... 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?

marianatandon commented 2 years ago

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.

dfernandez-asapp commented 2 years ago

@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:

villasv commented 2 years ago

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).

dfernandez79 commented 2 years ago

@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:

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 commented 2 years ago

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

lecstor commented 2 years ago

@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.