Exported components should not include dependencies

[soedar]

When troubleshooting an issue when using the library with a Next.js application, I noticed that the code exported by the library appears to include its dependencies. This may result in a shadowing behavior, where the dependencies being bundled by the library is used instead of the one installed by npm.

Reproducing behavior

# Create new Next.js app, and install sgds-react library
npx create-next-app@latest
cd my-app
npm install @govtechsg/sgds-react @govtechsg/sgds bootstrap-icons
cd node_modules/@govtechsg/sgds-react

# See which files include the following line
grep -l "When server rendering, you must wrap your application in an <SSRProvider> to ensure consistent ids are generated between the client and server" */*.js

# Output
# We see that this line is being included in multiple component. 
# The line appears to come from the react-aria SSRProvider library.
Combobox/index.js
DatePicker/index.js
Dropdown/index.js
Nav/index.js
SplitButton/index.js
Tabs/index.js
cjs/index.js

Proposed changes

When packaging the library as a module for other projects, it seems like we should let npm handle the dependencies that would be used by the project and all other libraries. As such, we should not bundle the dependencies when generating the code for components. In addition, we should probably not minify/uglify the output, as it would make troubleshooting issues on the server side more difficult.

In addition, we can also generate a bundled JavaScript (iife), that could be used in the browser. In that case, we would need to include all the bundled dependency (less peer dependencies), and also minify/uglify the output. Essentially, this means we can provide a JS file that can be imported on the browsers (similar to this: https://react-bootstrap.github.io/docs/getting-started/introduction#browser-globals)

I propose the following changes:

1. Update rollup to not include all node_modules dependencies when generating code for each component and the main bundle.
2. Add rollup config to generate a bundle with all components and all the dependencies (less peerDependencies), which can be used directly on the browser.

If this sounds ok, I can work on both changes, and would open PRs for review.

[clukhei]

Hi @soedar, yes I agree that we should not be bundling and minifying the output.

Could you elaborate on point 1 on how you plan on going about that? I am assuming that you will be also be using preserveModules and preserveModulesRoot to also preserve the current entry points we already have?
Also, with your changes do you foresee any breaking changes ?

[soedar]

Could you elaborate on point 1 on how you plan on going about that? I am assuming that you will be also be using preserveModules and preserveModulesRoot to also preserve the current entry points we already have?

Ah, that is interesting. I would look into that.

It seems like the current rollup configuration treats each component as a separate "build", and the package.json for each component is generated using the generatePackageJson plugin.

In my (limited) testing, I was able to generate component outputs that does not include node_modules dependencies simply by changing the the plugins that are used. However, the component output still included code of other components (i.e. when if a component include ../ThemeProvider, the ThemeProvider code is bundled together with the component)

But good find, I'll see if I can tweak the config based on preserveModules and preserveModulesRoot

Also, with your changes do you foresee any breaking changes ?

So far during my testing, it seems like there shouldn't be any breaking changes, since we are only changing the way the modules are being generated. But will probably know more once we arrive at the solution and testing possible edge cases.

[soedar]

Looking at the rollup docs for preserveModules,

It is therefore not recommended to blindly use this option to transform an entire file structure to another format if you directly want to import from those files as expected exports may be missing. In that case, you should rather designate all files explicitly as entry points by adding them to the input option object, see the example there for how to do that.

Since we want to have explict entry points for each of the components, it seems like we should continue to use the current rollup configuration, where the different components are supplied as inputs to rollup.

[clukhei]

Rollup changes released in v2.2.2 https://github.com/GovTechSG/sgds-govtech-react/releases/tag/v2.2.2

Thank you @soedar for your contributions🌟, I shall close this thread now