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

Include ES modules in NPM package #1300

Merged
merged 15 commits into from
May 8, 2018

Conversation

interactivellama
Copy link
Contributor

@interactivellama interactivellama commented Mar 2, 2018

Fixes #1284. Fixes #1269. Fixes #1265.

Additional description

Final solution was ES5 code wrapped in ES6 modules with package.sideEffects = false. This allows Webpack 4 to tree shake.

Allows package.module to link to ES6 modules. This has been tested in the following ways:

Uses ES6 modules package in:
Create React App with Webpack 3.

Does tree shaking with
Webpack 4

CRA 2.0 should use Webpack 4 and do the tree shaking.


Pull Request Review checklist (do not remove)

  • Run npm run dist and inspect files
  • Copy/paste .tmp-npm into an existing project to see if it works.

@interactivellama
Copy link
Contributor Author

Looks like Karma needs CommonJS babel plugin to run tests now that it's removed from .babelrc. Looking into.

@interactivellama
Copy link
Contributor Author

I'm going to step back from this task for a week or so. All that needs to be done is to have the code that goes into Karma/PhantomJS to be CommonJS->UMD and ready for the browser.

There's probably a way to configure webpack with ENV vars or turn Babel settings back into modules: commonJS which was just changed in @salesforce/babel-preset-design-system-react@2.0 to modules: false if someone else once to pick this up in order to create compatibility with Create React App.

https://github.com/salesforce/design-system-react/pull/1300/files#diff-a34975409803f73a0edc00817a158877R12

@interactivellama
Copy link
Contributor Author

@davidlygagnon has offered to look into this issue.

@interactivellama
Copy link
Contributor Author

interactivellama commented Apr 5, 2018

I'm thinking that it may just be easier to run the build/release process with preset.module turned on and revert the preset https://github.com/salesforce/design-system-react/blob/master/.babelrc#L2 back to CommonJS, so that folks can decide what kind of module bundling they want to do.

This way we don't have to re-config our test runner right now.

Here is the ES6 module build CLI command:
https://github.com/salesforce/design-system-react/pull/1300/files#diff-e55923752497a0f056171f423eb1f9d2R117

@davidlygagnon
Copy link
Contributor

Yeah, that's another option. How would consumer use preset.module ? In this case, we won't build a folder with ES6 code anymore?

@interactivellama
Copy link
Contributor Author

We will still build a folder of ES6 modules, we just have to have Babel in ES6 output module when we build them. This can be done by overriding the module key during the ES6 build.

Now that I've had it in the back of my mind for weeks, the problem with setting Babel to export as ES6 modules instead of CommonJS is that whatever consumes the post-Babel code has to understand ES6 modules--which is fine for most Webpack configs, but we are kind of dictating that with the preset in v2.0, and I think CommonJS as the default (consumers can always override a preset) which probably be better for the foreseeable future.

@interactivellama
Copy link
Contributor Author

To directly answer your question, the preset.module flag is for output format only.

@interactivellama
Copy link
Contributor Author

@davidlygagnon I'm transferring this issue to @futuremint since he said he was familiar with Bable transforms, etc.

@futuremint We will need to release a 3.0 version of the babel preset that reverts module key back to undefined. Just use the 1.0 for now--I think, and we can bump the preset NPM module when this issue is merged. https://www.npmjs.com/package/@salesforce/babel-preset-design-system-react

@davidlygagnon
Copy link
Contributor

Sure no worries. Sorry, haven't had much time to contribute in the last few weeks.. Hopefully soon.

@futuremint
Copy link
Contributor

futuremint commented Apr 24, 2018

@interactivellama ok this is ready to review now. I tested this in Webpack 4 using the following minimal config:

const path = require('path');

module.exports = {
    mode: 'production',
    module: {
        rules: [
            {
                test: /\.jsx?$/,
                include: [path.resolve(__dirname)],
                use: { loader: 'babel-loader' }
            }
        ]
    },
    resolve: {
        extensions: ['.js', '.jsx'],
        alias: { 'design-system-react': path.resolve(__dirname, 'node_modules/@salesforce/design-system-react') }
    },
};

And this as the JS file:

import { Button, Icon, DataTable, PageHeader } from 'design-system-react';

// All commented out, bundle is: 566 bytes.
Icon(); // just this is 34.2KB 
// Button(); // just this is 211KB
// DataTable(); // just this is 279KB
// PageHeader(); // just this is 221KB
// But all uncommented are 293KB

These results required the addition of sideEffects: false to the package.json file in the npm package. The reason being that all of the re-exports in our big index.js file don't automatically get trimmed without it. Here is the relevant Webpack documentation.

I put my example repo here.

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.

3 participants