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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Centered: remove typesVersions attribute #9907

Merged
merged 2 commits into from Feb 20, 2020

Conversation

@gaetanmaisse
Copy link
Member

@gaetanmaisse gaetanmaisse commented Feb 19, 2020

Issue

After testing #9847 on a real angular project I faced the following error:

TS2307: Cannot find module '@storybook/addon-centered/angular'.

After 馃攳 it looks like it's due to the fact that this addon is exporting handwritten types from the addon rrot dir but TS was looking for types inside dist or ts3.5/dist folder.

What I did

Removes typesVersions attribute because this addon exports handwritten types from base dir and so we should still let TS uses them instead of the one from dist or ts3.5/dist folder.

鈿狅笍 Be careful when writing types by hand, they should be compatible with TS3.5

鈥orts hand written types from base dir

Hand written types are inside addon base dir and so we can not use `typesVersions`
鈿狅笍 Be careful when writing types by hand, they should be compatible with TS3.5
@lychyi
Copy link
Contributor

@lychyi lychyi commented Feb 19, 2020

@gaetanmaisse This may be because the dowlevel-dts in the compile-tsc.js script isn't expecting types to be outside of dist. This was an oversight on my part, I didn't know there were other packages that had .d.ts outside of dist but I see what's happening now.

I think we should just downlevel all d.ts files. There are a few other things that I'm realizing that was missed in #9847.

  • cleanup script
  • ts3.5 -> gitignore

I'm on the next branch, I can take care of these things if you want and we can cherry pick it into master. Sorry about that!

@gaetanmaisse
Copy link
Member Author

@gaetanmaisse gaetanmaisse commented Feb 19, 2020

@lychyi Feel free to add commits to this branch/PR

@lychyi
Copy link
Contributor

@lychyi lychyi commented Feb 19, 2020

@gaetanmaisse Oh wait a minute 馃 I did dist specifically because without it, I was incorrectly downleveling typings.d.ts from src. So we would get something like ts3.5/src/typings.d.ts which wasn't what we wanted to ship.

I may need to think on this one. Removing typesVersions here still will ship ts3.5 to the package consumers. That downleveled directory wouldn't do anything but I think this fix would just be temporary no?

@gaetanmaisse
Copy link
Member Author

@gaetanmaisse gaetanmaisse commented Feb 19, 2020

True! So I think we can:

  • as a workaround for this addon: remove typesVersions but keep dist as input dir and ts3.5 as output dir (+ listed it in files) -> my commit
  • add ts3.5 in .gitignore
  • check the clean script to see if ts3.5 is deleted between to compilation

Ok with that?

@gaetanmaisse gaetanmaisse requested a review from igor-dv as a code owner Feb 19, 2020
@lychyi
Copy link
Contributor

@lychyi lychyi commented Feb 19, 2020

@gaetanmaisse Yes, if this is the only package that has d.ts files in the root let's just remove typesVersions for now to fix this bug.

What we can always do in the near future is downlevel all d.ts files and then delete ts3.5/src. I believe that would give us the expected outcome for root-level d.ts. This is assuming d.ts files can only exist in dist, src, and ./. If it exists anywhere else--which it shouldn't--it would make it into ts3.5. Not a deal breaker, just kind of messy.

@gaetanmaisse
Copy link
Member Author

@gaetanmaisse gaetanmaisse commented Feb 20, 2020

@lychyi have you tested that the base issue is fixed with the latest alpha?

@ndelangen ndelangen merged commit e04d5da into next Feb 20, 2020
48 of 49 checks passed
48 of 49 checks passed
@github-actions
Puppeteer & A11y tests Puppeteer & A11y tests
Details
@github-actions
Automention
Details
@github-actions
CLI Fixtures
Details
@github-actions
Danger JS
Details
@github-actions
CLI Fixtures with Yarn 2
Details
@github-actions
Latest CRA
Details
@netlify
Header rules No header rules processed
Details
@netlify
Pages changed All files already uploaded
Details
@netlify
Redirect rules No redirect rules processed
Details
@packtracker
packtracker/images No images assets found.
Details
@packtracker
packtracker/javascript 7.82 MB 鈥 No Change
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
Coverage (Storybook) TeamCity build finished
Details
@github-actions
Danger All green. Congrats.
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 379 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: 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
@ndelangen ndelangen deleted the fix-ts-export-for-addon-centered branch Feb 20, 2020
@lychyi
Copy link
Contributor

@lychyi lychyi commented Feb 20, 2020

@gaetanmaisse Hi Gaetan, sorry for the late response. I have not done testing on the latest alpha. Our repo is using 5.3 and would need some significant effort to adopt 6.0 right now. I could always help do other smoke tests but I probably won't be able to get to it until Saturday.

@gaetanmaisse
Copy link
Member Author

@gaetanmaisse gaetanmaisse commented Feb 20, 2020

@lychyi No pb, I think we are good to go 馃槈

@shilman can I let you cherry-pick/merge these commits to master? After this, I think we will be ok to release a 5.3.14

@shilman
Copy link
Member

@shilman shilman commented Feb 21, 2020

@gaetanmaisse sure i can do the picking if you're confident in the changes. can you label each PR you want picked with the patch label? 馃槄馃敨

@shilman shilman added the picked label Feb 25, 2020
shilman added a commit that referenced this pull request Feb 25, 2020
鈥ered

Centered: remove `typesVersions` attribute
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants