arqex / react-datetime

A lightweight but complete datetime picker react component.
1.99k stars 873 forks source link

Add a "browser" field so build tools can find UMD build #813

Open davidwallacejackson opened 2 years ago

davidwallacejackson commented 2 years ago

Description

Right now, the package.json only indicates the presence of the CommonJS build of react-datetime (as "main"). This change exposes the UMD build via the "browser" field.

Motivation and Context

Right now, Rollup loads the CJS version of react-datetime through @rollup/plugin-commonjs. That should work just fine, but there are some edge cases in the plugin -- this one in particular seems problematic for react-datetime.

I discovered all this by trying to import react-datetime in Vite and discovering that the library worked in development (where Vite implements its own esbuild-based CJS -> ESM transformation) but not in production (where Vite uses Rollup). Here's a minimal repro.

Including ESM directly would be ideal, but webpack still lists ESM output support as "experimental" in their docs, and I'm assuming this project doesn't particularly want to migrate to another build tool. I tried this out in my local copy and it fixed the issue.

Checklist

[x] I have not included any built dist files (us maintainers do that prior to a new release)
[ ] I have added tests covering my changes
[x] All new and existing tests pass
[ ] My changes required the documentation to be updated
  [ ] I have updated the documentation accordingly
  [ ] I have updated the TypeScript 1.8 type definitions accordingly
  [ ] I have updated the TypeScript 2.0+ type definitions accordingly
morlay commented 2 years ago

the umd could be the main.

davidwallacejackson commented 2 years ago

@morlay it could -- I'm not sure of the merits/appropriateness of using umd as the main entrypoint? I did this with "browser" because I found other projects doing it, and it fixed the issue I was having in Vite, but I have no strong reason to prefer this approach over the other.

fabianonunes commented 2 years ago

It may not be necessary to export the library in another format, although it is the best option. I think passing libraryExport="default" to webpack config is enough:

// config/webpack.config.build.js
const cjsConfig = {
    ...baseConfig,
    output: {
        path: outputPath,
        library: 'Datetime',
        libraryTarget: 'commonjs2',
        filename: 'react-datetime.cjs.js',
        auxiliaryComment: 'React datetime',
+               libraryExport: "default"
    }
};

This isn't a Vite issue but a @rollup/plugin-commonjs bug introduced by the refactoring done in rollup/plugins#537. To confirm, you can downgrade @rollup/plugin-commonjs to version 15.0.0.