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

Switch to Preconstruct for library building #520

Merged

Conversation

Andarist
Copy link
Contributor

Description

This PR switches away from tsup to preconstruct - a lesser-known but more capable tool.

The benefits are:

  • better cjs & esm setup
  • and types for the above (this should pass https://arethetypeswrong.github.io/ checks)
  • 'use client' directive support. With this you just can just add the directive to any of your files and Preconstruct takes care of isolating that in its own chunk
  • package.json#exports flexibility:
    • it should be possible to add extra entrypoints with ease if you ever need that (like next-cloudinary/something)
    • it's also easy to use differential bundling with this, if you ever need to ship different code for dev/prov bundles, or for node/browser/webworker/vercel's edge-light/younameit (or perhaps for react-server condition)

Issue Ticket Number

Related to #435

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Fix or improve the documentation
  • This change requires a documentation update

Checklist

  • I have followed the contributing guidelines of this project as mentioned in CONTRIBUTING.md
  • I have created an issue ticket for this PR
  • I have checked to ensure there aren't other open Pull Requests for the same update/change?
  • I have performed a self-review of my own code
  • I have run tests locally to ensure they all pass
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes needed to the documentation

Copy link

vercel bot commented Sep 25, 2024

@Andarist is attempting to deploy a commit to the Cloudinary DevX Team on Vercel.

A member of the Team first needs to authorize it.

Copy link

vercel bot commented Sep 26, 2024

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

Name Status Preview Updated (UTC)
next-cloudinary ✅ Ready (Inspect) Visit Preview Sep 26, 2024 6:34pm

@colbyfayock
Copy link
Collaborator

@Andarist one clarifying thing, so this doesn't set up subpath imports, correct? this just bundles a bit differently so that we can successfully apply the directive in the relevant components?

what impact does this have on tree-shaking, does this improve? maintain the same? i thought we had treeshaking covered but a comment in another Issue made it sound like that wasn't the case

i'm also curious about if it impacts the size, but ill test this out locally too

@colbyfayock
Copy link
Collaborator

i also noticed that the components are now available as separate files. does this open up the possibility for someone to import next-cloudinary/cldimage? it may be out of scope here, but for those who want that, if its even necessary pending the treeshaking question above, could be a nice bonus

image

@colbyfayock
Copy link
Collaborator

is there a reason the components get their own files but the helpers dont? jsut curious about how that works in practice. what is determined to split into its own chunk?

@Andarist
Copy link
Contributor Author

@Andarist one clarifying thing, so this doesn't set up subpath imports, correct?

Yes, it doesn't introduce any extra entrypoints but they can easily be done by:

  • adding entrypoints to the config, like here
  • running preconstruct fix

With Preconstruct package.json#exports is the output - you don't have to worry about its structure. In fact, preconstruct build fails if package.json#exports shape doesn't match your configuration and requests you to run preconstruct fix to regenerate this output. Thanks to that, it makes sure that package.json#exports are defined correctly for the whole matrix of builds that can be generated (which would be hard and error-prone to do by hand)

this just bundles a bit differently so that we can successfully apply the directive in the relevant components?

When it comes to file-splitting - yes, this is the main difference here. It ensures that all files with the 'use client' directive stay isolated and that allows the RSC-aware tools (like Next) to handle it correctly.

what impact does this have on tree-shaking, does this improve? maintain the same?

It's roughly the same.

i thought we had treeshaking covered but a comment in another Issue made it sound like that wasn't the case

I'll prepare this week a breakdown of how tree-shaking performs with this setup and recheck if anything can be improved. I feel like people usually misread some stats and get imprecise takeaways based on them. Tree-shaking is actually quite simple algorithm and the rules are pretty simple. At the same time, there are various optimizations at play and people might often refer to all of them combined as tree-shaking so it's often hard to answer questions like this without analyzing how it actually plays out for any given library/situation. Once I'm done with the writeup, I can try to answer followup questions to clear up how it works in practice and what kind of rule of thumbs you could stick to to ensure that it doesn't regress in the future etc.

i'm also curious about if it impacts the size, but ill test this out locally too

It should roughly be the same for consumers. The install size also shouldn't be affected in a meaningful way. This doesn't use any extra builds beyond just ESM and CJS and you already had those 2 here.

i also noticed that the components are now available as separate files. does this open up the possibility for someone to import next-cloudinary/cldimage?

No. package.json#exports is the full manifest of what paths can be imported from the package. So when it comes to JS files, this now only allows next-cloudinary, all next-cloudinary/dist/**.js files are forbidden. Like I've mentioned above, it wouldn't be hard to add extra entrypoints but, personally, I don't see a big value in that here. I think it's enough to rely on tree-shaking and keep a single entrypoint. YMMV though so if you feel like you want those, I can add them. I'd also gladly set up a call to discuss how I see this and discuss tradeoffs etc.

is there a reason the components get their own files but the helpers dont?

Components relying on hooks need to be isolated so they stay in files with 'use clent'. Traditionally it was better to put as much of the code into "flat bundles" because:

  • it was hiding the internal directory structure from the users, ensuring they don't start accidentally relying on paths that were not meant to be imported from externally
  • certain tree-shaking algorithms could produce better results in flat bundles (at the very least webpack 4 performed better)

Those are not that important in the today's landscape though. The first concern is addressed by package.json#exports - that hides the whole internal directory structure from the users already. The second concern might feel outdated, modern tools should perform OK even when code is split across files. I haven't done a comparison of this aspect of the current tools in a while though.

Flat bundling is Rollup's default so we just kinda stick to it when possible. It's also nice that the module resolver doesn't always have to resolve a bunch of relative imports here but that's a microoptimization at best in the grand scheme of things :p

@colbyfayock colbyfayock changed the base branch from main to beta September 26, 2024 18:07
@colbyfayock colbyfayock merged commit a35a928 into cloudinary-community:beta Sep 27, 2024
4 checks passed
@colbyfayock colbyfayock mentioned this pull request Sep 27, 2024
github-actions bot pushed a commit that referenced this pull request Sep 27, 2024
# [7.0.0-beta.1](v6.13.0...v7.0.0-beta.1) (2024-09-27)

### Features

* Switch to Preconstruct for library building ([#520](#520)) ([a35a928](a35a928))

### BREAKING CHANGES

* New build tool, no longer requires `use client` directive, marking as breaking for safety
Copy link

🎉 This PR is included in version 7.0.0-beta.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants