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

Add downlevel dts master #9847

Merged
merged 4 commits into from Feb 18, 2020
Merged

Conversation

@lychyi
Copy link
Contributor

@lychyi lychyi commented Feb 13, 2020

Issue: Fixes #9463, an alternative to #9826

While there are incompatible declaration outputs between TypeScript 3.7+ and TS3.5, there is a workaround to use the latest TypeScript version while still maintaining compatible d.ts files. This is accomplished by using downlevel-dts to generate alternative typings for users on a certain version of TypeScript.

In this PR, any users who are on TS3.5 and below, TypeScript will automatically look at the package.json for a field called typesVersions and utilize the typings in that folder via a mapping.

  "typesVersions": {
    "<=3.5": { "*": ["ts3.5/*"] }
  },

What I did

I modified the compile-tsc.js script to use downlevel-dts and generate typings from the dist folder to a ts3.5/dist folder. Then I added the above snippet to each package.json with a corresponding types field. I'm assuming other package.jsons without types are not actually exporting any TypeScript files.

Refer to the comments in #9463 for more detail.

How to test

I tested it by manually altering node_modules with this output in @storybook/channels inside my own project that suffers from #9463. The project indeeds builds correctly now with TypeScript 3.5.3 and Storybook 5.3.12.

I also verified that the folder lib/channels/ts3.5/dist/index.d.ts has this line:

readonly hasTransport: boolean;

instead of

get hasTransport(): boolean;

I'd probably like a second set of eyes on it to make sure it works and also help out on any automated tests(if we think it's worth the effort).

@lychyi lychyi force-pushed the lychyi:add-downlevel-dts-master branch from 00bb321 to 038b7f9 Feb 13, 2020
Copy link
Member

@gaetanmaisse gaetanmaisse left a comment

You have updated package.json of all libs and addons, I think we need to do the same for all /apps packages that are written in TS.

scripts/compile-tsc.js Show resolved Hide resolved
addons/a11y/package.json Outdated Show resolved Hide resolved
@gaetanmaisse
Copy link
Member

@gaetanmaisse gaetanmaisse commented Feb 14, 2020

@lychyi Thanks for this PR 😍 great work! 👍
We just need to discuss the output folder #9847 (comment) ;)

@gaetanmaisse gaetanmaisse self-assigned this Feb 14, 2020
@shilman
Copy link
Member

@shilman shilman commented Feb 14, 2020

Y'all are so on top of this stuff. Great work!!! 🙇🙇🙇

@lychyi
Copy link
Contributor Author

@lychyi lychyi commented Feb 14, 2020

@gaetanmaisse Good points, I just followed the example on downlevel-dts so I had to think about it for a sec. This was the most compelling reason I could think of:

ts3.5/dist is essentially mirroring whatever was in dist in another location. If we do dist/ts3.5, in the very low chance that someone names a src folder ts3.5 you'll essentially clobber what's in there when you compile and downlevel TS into dist. So the downside in this case is that ts3.5 becomes a reserved folder name that you can't use, it's not a deal breaker but it's just a mental nugget that you have to keep in mind.

And yes, good call on the files attribute 😅... and setting typesVersions for app/. I did a find and replace for the "types: " attribute in package.json files and I'm baffled how I missed that.

It probably is worth it to say though, I'll defer to the maintainers because that's the biggest impact as a result of this question. I don't have a strong preference.

@lychyi lychyi force-pushed the lychyi:add-downlevel-dts-master branch from 038b7f9 to 22c067e Feb 14, 2020
@lychyi lychyi requested review from saponifi3d and UsulPro as code owners Feb 14, 2020
@gaetanmaisse
Copy link
Member

@gaetanmaisse gaetanmaisse commented Feb 15, 2020

@ndelangen @kroeder what do you think about downlevel-dts' output folder ⬆️ #9847 (comment) and #9847 (comment)?

@lychyi
Copy link
Contributor Author

@lychyi lychyi commented Feb 18, 2020

✏️ Just writing down a mental note here ✏️

This PR still may need the files attribute altered in package.json before merging. I'll do it once the decision is made about where we downlevel things to.

@lychyi lychyi force-pushed the lychyi:add-downlevel-dts-master branch from 22c067e to f0edad2 Feb 18, 2020
@lychyi
Copy link
Contributor Author

@lychyi lychyi commented Feb 18, 2020

@gaetanmaisse files attribute has been updated.

Note: I added this to every package.json that specifies that it ships *.d.ts in files even though the package may not have d.ts files to ship. I'm still debating if this is the right call but I figured if we're specifying that it could ship d.ts files in the future, we would need to ship downleveled types too so I made it consistent across the project so we don't have to account for it in the future.

For example, if someone wants to create a new package, do they copy and paste from an existing package.json or is there a script that generates one? If it's the former, then having it in every package.json would make sense.

Maintainers: feel free to modify the PR if that wasn't the right call or I can do it as soon as you let me know.

@gaetanmaisse
Copy link
Member

@gaetanmaisse gaetanmaisse commented Feb 18, 2020

@lychyi LGTM! I just added a commit with some typesVersions you missed. Or was it intentional?

Tested locally:

  • Reproduce #9463
  • Use packages of this branch with yarn link
  • Everything was 🟢

@shilman @ndelangen don't know if someone else wants to take a look 👨🏻‍💻 If not, I think it can be merged, what's the process, merge on master and then master is merged in next? or merged on next and then cherry-picked on master?

@ndelangen
Copy link
Member

@ndelangen ndelangen commented Feb 18, 2020

@gaetanmaisse This PR targets master, so let's merge this to master first

I'm unsure if cherry-picking this is easier then creating an additional PR against next?

@lychyi
Copy link
Contributor Author

@lychyi lychyi commented Feb 18, 2020

@gaetanmaisse Ah, I think I wrongly assumed that only packages with the types attribute shipped typings to downlevel. Good call on this.

@gaetanmaisse gaetanmaisse merged commit 82e2caa into storybookjs:master Feb 18, 2020
44 of 46 checks passed
44 of 46 checks passed
@github-actions
Automention
Details
@github-actions
Danger JS
Details
@packtracker
packtracker/images Awaiting your base commit report...
Details
@packtracker
packtracker/javascript Awaiting your base commit report...
Details
@netlify
Header rules No header rules processed
Details
@netlify
Pages changed 17 new files uploaded
Details
@netlify
Redirect rules No redirect rules processed
Details
Aggregate Examples (Examples) TeamCity build finished
Details
Build (Storybook) TeamCity build finished
Details
Chromatic (Storybook) TeamCity build finished
Details
Chromatic 1 (Chromatic) TeamCity build finished
Details
Chromatic 2 (Chromatic) TeamCity build finished
Details
Chromatic 3 (Chromatic) TeamCity build finished
Details
Chromatic 4 (Chromatic) TeamCity build finished
Details
CodeFactor No issues found.
Details
Coverage (Storybook) TeamCity build finished
Details
DeepScan 0 new and 0 fixed issues
Details
Docs (Storybook) TeamCity build finished
Details
E2E (Storybook) TeamCity build finished
Details
Examples 1 (Examples) TeamCity build finished
Details
Examples 2 (Examples) TeamCity build finished
Details
Examples 3 (Examples) TeamCity build finished
Details
Examples 4 (Examples) TeamCity build finished
Details
Examples 5 (Examples) TeamCity build finished
Details
@netlify
Mixed content No mixed content detected
Details
Packtracker (Storybook) TeamCity build finished
Details
Smoke Tests (Storybook) TeamCity build finished
Details
Test (Storybook) TeamCity build finished
Details
Test Workflow (Storybook) TeamCity build finished
Details
ci/chromatic 381 stories unchanged.
Details
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: chromatic Your tests passed on CircleCI!
Details
ci/circleci: coverage Your tests passed on CircleCI!
Details
ci/circleci: docs Your tests passed on CircleCI!
Details
ci/circleci: e2e Your tests passed on CircleCI!
Details
ci/circleci: examples Your tests passed on CircleCI!
Details
ci/circleci: frontpage Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: native-smoke-tests Your tests passed on CircleCI!
Details
ci/circleci: packtracker Your tests passed on CircleCI!
Details
ci/circleci: smoke-tests Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@circleci-checks
deploy Workflow: deploy
Details
@netlify
deploy/netlify Deploy preview ready!
Details
@circleci-checks
test Workflow: test
Details
@lychyi lychyi deleted the lychyi:add-downlevel-dts-master branch Feb 18, 2020
@gaetanmaisse
Copy link
Member

@gaetanmaisse gaetanmaisse commented Feb 18, 2020

@lychyi I think it was a valid assumption but there are suspicious things with these packages I will take a look at them 🔍🕵

@gaetanmaisse
Copy link
Member

@gaetanmaisse gaetanmaisse commented Feb 18, 2020

@lychyi Thanks for your work 🎉 🎉 🙇

I will cherry-pick and create the PR for next ASAP.

@lychyi
Copy link
Contributor Author

@lychyi lychyi commented Feb 18, 2020

🎉Thanks for the assist! ❤️ Lovin' the project and are more than happy to help. @gaetanmaisse @ndelangen @shilman

@shilman
Copy link
Member

@shilman shilman commented Feb 18, 2020

@lychyi @gaetanmaisse @ndelangen I just realized that this was merged into master and the second PR is going into next! 🙀

I just released the second PR as part of https://github.com/storybookjs/storybook/releases/tag/v6.0.0-alpha.14

Since this feels like a biggish change, can you please QA that release a bit and let me know if it looks good? Once I get your go-ahead, I'll release this change on 5.3.x.

Thanks!! 🙏🙏🙏

@gaetanmaisse
Copy link
Member

@gaetanmaisse gaetanmaisse commented Feb 19, 2020

🛑 It looks like there is a bug with Addon Centered because it's the only package that export types from "root" dir. Here is the fix: #9907 ⚠️ I targeted next

Everything else looks good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment