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

Build libs/router with ts-up #19140

Merged
merged 5 commits into from
Sep 8, 2022

Conversation

hayawata3626
Copy link
Member

Issue: #18732

What I did

Use tsup to build router

How to test

  • Is this testable with Jest or Chromatic screenshots?
  • Does this need a new example in the kitchen sink apps?
  • Does this need an update to the documentation?

If your answer is yes to any of these, please make sure to include it in your PR.

@IanVS IanVS added maintenance User-facing maintenance tasks core labels Sep 8, 2022
@IanVS
Copy link
Member

IanVS commented Sep 8, 2022

Thanks for the PR, @hayawata3626! A few notes:

  • I'll take care of the DeepScan issue in a different PR, not sure why it started failing suddenly.
  • To solve the other error in CI, try adding "platform": "browser" to the "bundler" config in package.json.
  • Lastly, you can remove core-js (and re-run yarn install to update yarn.lock) now that babel isn't being used to compile this package.

@hayawata3626
Copy link
Member Author

hayawata3626 commented Sep 8, 2022

@IanVS
Thanks for some notes!
I understand and will try!

@hayawata3626
Copy link
Member Author

There did not seem to be any core-js in lib/router.

@IanVS
Copy link
Member

IanVS commented Sep 8, 2022

There did not seem to be any core-js in lib/router.

Whoops, okay, thanks, I thought there was!

@IanVS
Copy link
Member

IanVS commented Sep 8, 2022

Alright, I'm not sure what is causing the error now. Maybe @ndelangen will know.

@ndelangen ndelangen self-requested a review September 8, 2022 13:17
@ndelangen
Copy link
Member

This is great, I think that was the last package using my old prebundling custom util.
I'll remove the code in a follow up PR. Unless you want to do that @hayawata3626 ?

Thanks, I have a good feeling that this will work now.

@ndelangen ndelangen self-assigned this Sep 8, 2022
@hayawata3626
Copy link
Member Author

hayawata3626 commented Sep 8, 2022

@ndelangen
Thank you for your reply!

I'll remove the code in a follow up PR

Should I just delete utils.js in the libs/router in this PR ? I would like to try!

@ndelangen
Copy link
Member

I'm not sure what utils.js is used for, but I think it's important. What I am referring to is the code in ../../../scripts/prebundle.ts. That is a tool I custom made because I could not use ts-up at the time, because it required TypeScript v4, and we didn't think we'd be able to upgrade any time soon.

@hayawata3626
Copy link
Member Author

hayawata3626 commented Sep 8, 2022

@ndelangen
Thank you for the explanation.
I will identify and delete files related to ../../../scripts/prebundle.ts.
Is this PR okay? Or another PR?

@ndelangen ndelangen merged commit 0b15ba3 into storybookjs:next Sep 8, 2022
@ndelangen
Copy link
Member

It sure is! 🎉

@ndelangen
Copy link
Member

Thank you so much for the help @hayawata3626 ❤️ !!

@hayawata3626
Copy link
Member Author

hayawata3626 commented Sep 8, 2022

@ndelangen
I would like to contribute further!!

@hayawata3626 hayawata3626 deleted the add-build-setting-router branch September 8, 2022 14:00
@ndelangen
Copy link
Member

I invited you to the org 🎈

@hayawata3626
Copy link
Member Author

@ndelangen
I‘m so happy to be a member of storybook!! Thank you!!

@ndelangen
Copy link
Member

Enjoy the free code co-pilot 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core maintenance User-facing maintenance tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants