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

convert to tsup #2652

Merged
merged 33 commits into from
Sep 30, 2023
Merged

convert to tsup #2652

merged 33 commits into from
Sep 30, 2023

Conversation

shahriar-shojib
Copy link
Collaborator

@shahriar-shojib shahriar-shojib commented Sep 28, 2023

Description
implements #2627

checklist:

  • migrate to tsup
  • make sure packages can be watched
  • make sure nextjs hot reloads if a package is changed
  • keep bundle format as it was on rollup
  • deployments passing
  • tests passing
  • test for @swc/core regressions
  • performant watching of all packages

/claim #2627

keeping this as draft so that I can run some tests

See changesets.

@changeset-bot
Copy link

changeset-bot bot commented Sep 28, 2023

🦋 Changeset detected

Latest commit: a0d56f4

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 56 packages
Name Type
@udecode/plate-alignment Minor
@udecode/plate-autoformat Minor
@udecode/plate-basic-elements Minor
@udecode/plate-basic-marks Minor
@udecode/plate-block-quote Minor
@udecode/plate-break Minor
@udecode/plate-caption Minor
@udecode/plate-ui Minor
@udecode/plate-cloud Minor
@udecode/plate-code-block Minor
@udecode/plate-combobox Minor
@udecode/plate-comments Minor
@udecode/plate-common Minor
@udecode/plate-core Minor
@udecode/plate-cursor Minor
@udecode/plate-dnd Minor
@udecode/plate-emoji Minor
@udecode/plate-excalidraw Minor
@udecode/plate-find-replace Minor
@udecode/plate-floating Minor
@udecode/plate-font Minor
@udecode/plate-heading Minor
@udecode/plate-highlight Minor
@udecode/plate-horizontal-rule Minor
@udecode/plate-indent Minor
@udecode/plate-indent-list Minor
@udecode/plate-juice Minor
@udecode/plate-kbd Minor
@udecode/plate-line-height Minor
@udecode/plate-link Minor
@udecode/plate-list Minor
@udecode/plate-media Minor
@udecode/plate-mention Minor
@udecode/plate-node-id Minor
@udecode/plate-normalizers Minor
@udecode/plate-paragraph Minor
@udecode/plate Minor
@udecode/plate-utils Minor
@udecode/plate-reset-node Minor
@udecode/plate-resizable Minor
@udecode/plate-select Minor
@udecode/plate-selection Minor
@udecode/plate-serializer-csv Minor
@udecode/plate-serializer-docx Minor
@udecode/plate-serializer-html Minor
@udecode/plate-serializer-md Minor
@udecode/slate Minor
@udecode/slate-react Minor
@udecode/slate-utils Minor
@udecode/plate-suggestion Minor
@udecode/plate-tabbable Minor
@udecode/plate-table Minor
@udecode/plate-test-utils Minor
@udecode/plate-trailing-block Minor
@udecode/utils Minor
@udecode/plate-yjs Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@vercel
Copy link

vercel bot commented Sep 28, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
plate ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 30, 2023 2:58pm

@reviewpad
Copy link
Contributor

reviewpad bot commented Sep 28, 2023

Thank you @shahriar-shojib for this first contribution!

@reviewpad reviewpad bot added the large Pull request is large label Sep 28, 2023
config/tsup.config.ts Outdated Show resolved Hide resolved
config/tsup.config.ts Outdated Show resolved Hide resolved
cjsInterop: true,
dts: true,
replaceNodeEnv: true,
skipNodeModulesBundle: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this required?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this will skip bundling of anything imported that is not local to the package even if it is a dependency.
Ideally, for packages like these, I think it's best not to bundle node_module packages in the build files since it will be taken care of by the end user's bundler.
Since we're not shipping browser bundles this provides some safety in my opinion.
If you think this is unnecessary I can remove it

Copy link
Member

Choose a reason for hiding this comment

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

We will test it on next release

config/tsup.config.ts Outdated Show resolved Hide resolved
@shahriar-shojib
Copy link
Collaborator Author

I am not sure why the preview builds are failing, and I am unable to check the vercel logs.

@shahriar-shojib
Copy link
Collaborator Author

I am unsure if tailwind, babel, and JSON to esmodule support are necessary for the builds.
As I understand, these things are required by the plateUI components and those are no longer packaged up.

@zbeyens
Copy link
Member

zbeyens commented Sep 28, 2023

Tailwind and json are not. If there is no regression without Babel, we could proceed without it. Note that yarn test should still work.

The CI that is failing is yarn build && yarn workspace www build which you can run locally.

@shahriar-shojib
Copy link
Collaborator Author

I was able to run the test locally and it passed, CI logs are not showing why exactly the tests are failing, I will dig into it.
As for yarn workspace www build it also completes locally without any issues.

Could I get some logs from Vercel, please?

@zbeyens
Copy link
Member

zbeyens commented Sep 28, 2023

2023-09-28 at 18 36 20

I think the reason why it works locally is that you already have a build somehow. Try to run yarn g:clean.

@shahriar-shojib
Copy link
Collaborator Author

SCR-20230928-ueup SCR-20230928-ugta SCR-20230928-uiby

I ran all of the commands using --force to bypass turbo cache all of them succeeded on local, I ran yarn g:clean before running these.
I am currently checking if there's any cache at play here that's causing the tests to fail on CI.

@shahriar-shojib
Copy link
Collaborator Author

ok so, I was able to get it run completely after adjusting a few values on concurrency, it was timing out on my repo.

Here's what I suspect is going on:

I think turbo runs tasks with parallel task count equal to host logical cores, and jest runs tests in parallel which equals to host logical CPU count too.

I have some further adjustments to make to make sure we're using the system to its fullest potential.

Also, what are your thoughts on using @swc/jest ?
I changed the jest transformer from ts-jest to @swc/jest on local and saw a massive improvement in test duration.
if you want I can make the necessary changes and push it, as per my early testing, all tests seem to run and pass.
Since we have a typecheck step in CI I think not checking the types for tests will be a big issue.

my test run: https://github.com/shahriar-shojib/plate/actions/runs/6343411359

@shahriar-shojib
Copy link
Collaborator Author

as for vercel deployment I am unable to figure out why it's failing, as you can see from the CI outputs it does not fail at the build step.

@zbeyens
Copy link
Member

zbeyens commented Sep 28, 2023

Thanks for suggesting @swc/jest, tests were a bit slow so let's use it.

As for Vercel, there is no error except
2023-09-28 at 23 27 43
It's probably a concurrency issue. Could you try using https://vercel.com/docs/cli/build locally?

switch to @swc/core
nextjs conditional config
revert ci edits
@shahriar-shojib shahriar-shojib marked this pull request as ready for review September 30, 2023 09:53
@zbeyens
Copy link
Member

zbeyens commented Sep 30, 2023

@zbeyens
Copy link
Member

zbeyens commented Sep 30, 2023

I have fixed the is-hotkey bug. Please rebase on same branch.

@shahriar-shojib
Copy link
Collaborator Author

This looks nice, but I just added turbowatch and moved build:watch to use turbowatch

@shahriar-shojib
Copy link
Collaborator Author

please let me know if you need to build the dependents of a changed package

@zbeyens
Copy link
Member

zbeyens commented Sep 30, 2023

Excellent job, thanks! Turbowatch is doing exactly what we needed so you can remove tsx package and config/dev-packages.ts. Also, you can revert the AnyObject changes. Once done, I'll merge this and do a test release. Then we can test it out on templates/plate-playground.

As for building dependents, it's just about running yarn build in onChange: turbo caching system takes care of the rest. But let's keep what you did as I'm not sure we need to build the dependents since the packages are symlinked.

@shahriar-shojib
Copy link
Collaborator Author

I am getting this error when removing the type parameter.

image

convert AnyObject from interface to type
convert TElement from interface to type
convert TTest from interface to type
remove tsx package & dev script
@shahriar-shojib
Copy link
Collaborator Author

I fixed this by converting AnyObject, TElement, TTest from interface to type.

Should not cause any breaking changees.

@zbeyens zbeyens merged commit 4a99899 into udecode:main Sep 30, 2023
5 checks passed
@zbeyens zbeyens mentioned this pull request Sep 30, 2023
@shahriar-shojib
Copy link
Collaborator Author

shahriar-shojib commented Sep 30, 2023 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🙋 Bounty claim large Pull request is large
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants