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

Centered: remove typesVersions attribute #9907

Merged
merged 2 commits into from
Feb 20, 2020

Conversation

gaetanmaisse
Copy link
Member

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

…ports 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 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

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

@lychyi
Copy link
Contributor

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

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?

@lychyi
Copy link
Contributor

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

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

@ndelangen ndelangen merged commit e04d5da into next Feb 20, 2020
@ndelangen ndelangen deleted the fix-ts-export-for-addon-centered branch February 20, 2020 16:19
@lychyi
Copy link
Contributor

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

@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 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? 😅🔫

@gaetanmaisse gaetanmaisse added the patch:yes Bugfix & documentation PR that need to be picked to main branch label Feb 21, 2020
@shilman shilman added the patch:done Patch/release PRs already cherry-picked to main/release branch label Feb 25, 2020
shilman pushed a commit that referenced this pull request Feb 25, 2020
…tered

Centered: remove `typesVersions` attribute
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addon: centered bug patch:done Patch/release PRs already cherry-picked to main/release branch patch:yes Bugfix & documentation PR that need to be picked to main branch typescript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants