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

Replace rollup by tsup as building tool #2631

Closed
wants to merge 4 commits into from
Closed

Conversation

g1mishra
Copy link

@g1mishra g1mishra commented Sep 18, 2023

fix #2627
/claim #2627

Pull Request Description

  • Update the p:build scripts in the root package to use Tsup instead of Rollup.
  • Update the p:build:watch script in the root package to use Tsup with the --watch flag for automatic rebuilds on file changes.
  • Update g:build:watch to build and watch all packages concurrently (using turbo).
  • Enable hot reloading for the Next.js app www when changes occur in any file in packages, using transpilePackages. Currently, with Rollup, a restart is necessary. Modify the configuration to support hot reloading seamlessly. This is the main goal of that issue.
  • Ensure there are no regressions from the current Rollup configuration. The project should build and run without any issues and produce the same expected output as it does with Rollup. We will do a test release to confirm that one.

@changeset-bot
Copy link

changeset-bot bot commented Sep 18, 2023

⚠️ No Changeset found

Latest commit: 2c7fd7d

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

@vercel
Copy link

vercel bot commented Sep 18, 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 18, 2023 4:21pm

@reviewpad
Copy link
Contributor

reviewpad bot commented Sep 18, 2023

Thank you @g1mishra for this first contribution!

@reviewpad reviewpad bot added the large Pull request is large label Sep 18, 2023
@zbeyens zbeyens marked this pull request as draft September 18, 2023 06:31
@zbeyens
Copy link
Member

zbeyens commented Sep 18, 2023

Note that we should use https://nextjs.org/docs/app/api-reference/next-config-js/transpilePackages for the 4th spec

@g1mishra
Copy link
Author

@zbeyens, could you please explain how to make use of transpilePackages here?

Additionally, I'm interested in understanding any areas where I may have made mistakes in my current approach.

@zbeyens
Copy link
Member

zbeyens commented Sep 18, 2023

@g1mishra See https://github.com/itsjavi/turborepo-react-next as a reference.

@g1mishra
Copy link
Author

g1mishra commented Sep 18, 2023

@zbeyens

updates :

  1. I tried to use transpilePackages inside next.config.js but no success. It's not building packages on changes
    transpilePackages: ['@udecode/*'],

  2. Another way of doing it using turbo. I tried
    "dev": "concurrently \"turbo --filter=www dev\" \"yarn g:build:watch\""
    "g:build:watch": "turbo --filter \"./packages/**\" build:watch --concurrency 100",

It's working but it's watching for all the packages, so my device getting freezed

So to deal with the issue of watching all the packages, I have added turborepo-watch.mjs
This script look for the changes in package files, and on save it runs the command to build that package.

I'm not able to think of other ways of doing it.

@zbeyens
Copy link
Member

zbeyens commented Sep 18, 2023

Thanks for the update. I see the ref repo is using "workspace:*" for local packages. Before looking for another strategy, we need to validate any blocker – why it would work for the ref repo but not for Plate repo.

@rishi-raj-jain
Copy link
Contributor

Note that we should use https://nextjs.org/docs/app/api-reference/next-config-js/transpilePackages for the 4th spec

Wish that you told me that haha

@rishi-raj-jain
Copy link
Contributor

What's being done here is exactly what I did.

@rishi-raj-jain
Copy link
Contributor

Emotion: Saying respectfully!

@zbeyens
Copy link
Member

zbeyens commented Sep 19, 2023

No worries, the main focus should be on completing all the specifications required.

Comment on lines +22 to +24
const { dependencies = {}, peerDependencies = {} } = JSON.parse(
fs.readFileSync(path.join(cwd, 'package.json'), 'utf8')
);
Copy link
Contributor

Choose a reason for hiding this comment

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

this is not needed as tsup by default ignores all deps, including peerDeps.

Comment on lines +33 to +34
...Object.keys(dependencies),
...Object.keys(peerDependencies),
Copy link
Contributor

Choose a reason for hiding this comment

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

similar to my comment on the same earlier. this can be removed.

'react-textarea-autosize',
],
outExtension: (ctx) => {
return { js: `.${ctx.format === 'esm' ? 'es' : ctx.format}.js` };
Copy link
Contributor

Choose a reason for hiding this comment

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

this is incorrect, if you match the rollup output, you'd want to change this to:

js: ctx.format === 'esm' ? '.es.js' : '.js'

Copy link
Author

Choose a reason for hiding this comment

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

In roll-up config file output extension format is es.js for esm and cjs.js for cjs

Copy link
Contributor

@rishi-raj-jain rishi-raj-jain Sep 21, 2023

Choose a reason for hiding this comment

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

But for the build of Plate, if run rollup (basically clone, install and build), you'd see that cjs.js is not what you get. It's either es.js or .js.

Copy link
Author

Choose a reason for hiding this comment

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

yes I have checked it here in package.json of package

"main": "dist/index.js", "module": "dist/index.es.js",

Copy link
Contributor

Choose a reason for hiding this comment

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

@zbeyens
Copy link
Member

zbeyens commented Sep 27, 2023

I will close this pull request as stale. Thank you for your efforts!

@zbeyens zbeyens closed this Sep 27, 2023
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.

Replace rollup by tsup as building tool
3 participants