Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

types: leverage variadic tuple for plugin tuple #91

Merged
merged 1 commit into from
Aug 21, 2020

Conversation

ChristianMurphy
Copy link
Member

@ChristianMurphy ChristianMurphy commented Jun 27, 2020

when use([plugin, setting]) syntax is used, typescript can now warn if the settings object does not match with what the plugin expects.

⚠️ TypeScript 4 is currently in beta, this should not be merged until after there is a stable release.

@ChristianMurphy ChristianMurphy added 🦋 type/enhancement This is great to have 🧑 semver/major This is a change ☂️ area/types This affects typings 🌊 blocked/upstream This cannot progress before something external happens first labels Jun 27, 2020
@ChristianMurphy ChristianMurphy requested review from Rokt33r, Murderlon and a team June 27, 2020 19:51
@ChristianMurphy ChristianMurphy force-pushed the types/variadic-tuple-support branch from 67ef0cb to 882f7fa Compare June 27, 2020 19:51
@codecov-commenter

This comment has been minimized.

@wooorm
Copy link
Member

wooorm commented Jun 28, 2020

As I understand it, starting to use TS4 only features would break all TS3 users, correct?
If so: this sounds like the original issue we had when adding types. As our new types used TS3, folks on earlier TS versions who (even when deep down) depended on our projects, couldn’t compile anymore?

@ChristianMurphy
Copy link
Member Author

ChristianMurphy commented Jun 28, 2020

As I understand it, starting to use TS4 only features would break all TS3 users, correct?

It would break TS 3 users directly depending on unified's typings or who have strict lib checks enabled

If so: this sounds like the original issue we had when adding types. As our new types used TS3, folks on earlier TS versions who (even when deep down) depended on our projects, couldn’t compile anymore?

The frustration previously was from introducing a typescript version requirement/upgrade in a minor release.
This would introduce the change in a major release.
Which means version spreads ~ (patch), and ^ (minor) would not pick up these changes.

Combined a clear changelog, this allows downstream adopters to choose when they want to upgrade.
For most users TypeScript version upgrades are seamless, and using this update would be seemless.
Any errors thrown would be legitimate cases where the settings object don't match up with the plugin.

@ChristianMurphy
Copy link
Member Author

Another option on available.
It is possible to support multiple versions of the typings using https://www.typescriptlang.org/docs/handbook/declaration-files/publishing.html#version-selection-with-typesversions
We could publish a v3.2 and v4.0 version initially, and remove the v3.2 in a major release.
My concern would be the maintenance cost of having more than one version of the typings.

@wooorm
Copy link
Member

wooorm commented Jun 29, 2020

To me maintaining two versions for a bit doesn’t seem too bad. It allows us to push TS 4 earlier and to more folks (it doesn’t need to bubble through remark/rehype/retext). While the load of maintaining two versions shouldn’t be very time consuming (some copy/pasting, and we don’t touch them too much after creation). Then, say in a year or so, when a next major is due to some other reason, we can ditch TS 3.2?

@ChristianMurphy ChristianMurphy force-pushed the types/variadic-tuple-support branch from 882f7fa to 4f6ad66 Compare August 21, 2020 00:19
@ChristianMurphy
Copy link
Member Author

To me maintaining two versions for a bit doesn’t seem too bad. It allows us to push TS 4 earlier and to more folks (it doesn’t need to bubble through remark/rehype/retext). While the load of maintaining two versions shouldn’t be very time consuming (some copy/pasting, and we don’t touch them too much after creation).

Done, the types have been split.

@ChristianMurphy ChristianMurphy removed the 🌊 blocked/upstream This cannot progress before something external happens first label Aug 21, 2020
when `use([plugin, setting])` syntax is used, typescript can now warn if the settings object does not match with what the plugin expects.
@ChristianMurphy ChristianMurphy force-pushed the types/variadic-tuple-support branch from 4f6ad66 to b846694 Compare August 21, 2020 00:39
@ChristianMurphy ChristianMurphy requested a review from a team August 21, 2020 02:27
Copy link
Member

@Murderlon Murderlon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well done

@wooorm
Copy link
Member

wooorm commented Aug 21, 2020

Good to go? Minor?

@ChristianMurphy ChristianMurphy added 🧒 semver/minor This is backwards-compatible change and removed 🧑 semver/major This is a change labels Aug 21, 2020
@ChristianMurphy
Copy link
Member Author

Yep, good to go, and semver minor should be accurate.

@wooorm wooorm merged commit 7fc4271 into unifiedjs:main Aug 21, 2020
@ChristianMurphy ChristianMurphy deleted the types/variadic-tuple-support branch August 21, 2020 18:02
@wooorm
Copy link
Member

wooorm commented Aug 21, 2020

Released in 9.2.0!

@diegohaz
Copy link

diegohaz commented Sep 3, 2020

Not sure if it's related to this change or not, but just so you know, I got this error after upgrading to v9.2.0:

node_modules/rehype-react/types/index.d.ts:3:27 - error TS7016: Could not find a declaration file for module 'unified'. '/Users/haz/Projects/reakit/node_modules/unified/index.js' implicitly has an 'any' type.
  Try `npm install @types/unified` if it exists or add a new declaration (.d.ts) file containing `declare module 'unified';`

3 import {Transformer} from 'unified'
                            ~~~~~~~~~


Found 1 error.

I'm not using TS 4 yet. Solution was to use 9.1.0.

@ChristianMurphy
Copy link
Member Author

ChristianMurphy commented Sep 3, 2020

@diegohaz could you open a thread in https://github.com/unifiedjs/unified/discussions?
and share your tsconfig + package.json, or even better a runnable example of the issue in https://codesandbox.io ?

This change has been tested across typescript versions from 3.4 to 4.0 using dtslint, both in this repo, and again downstream in most remark/rehype plugins that provide typings (for example remark itself)
Quite a few cases are covered by the test suite, if this does turn out to be an issue, the details around tsconfig and surrounding tooling will be needed to track it down.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
☂️ area/types This affects typings 🧒 semver/minor This is backwards-compatible change 🦋 type/enhancement This is great to have
Development

Successfully merging this pull request may close these issues.

6 participants