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

Bundle UMD with rollup #159

Merged
merged 4 commits into from May 1, 2018
Merged

Bundle UMD with rollup #159

merged 4 commits into from May 1, 2018

Conversation

TrySound
Copy link
Contributor

@TrySound TrySound commented May 1, 2018

UMD is important as an easy way to use library via <script> tag.

In this diff I added rollup with size snapshot plugin to track final
size of the library with all dependencies (except react ones).

I replaced mixed imports with import * as React from 'react' since
they are being semantically incorrect are failed with rollup.

UMD is important as an easy way to use library via `<script>` tag.

In this diff I added rollup with size snapshot plugin to track final
size of the library with all dependencies (except react ones).

I replaced mixed imports with `import * as React from 'react'` since
they are being semantically incorrect are failed with rollup.
@FezVrasta
Copy link
Member

Thanks for the PR!

Mixed imports are valid syntax, they are described in the Mozilla MDN:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/import

@TrySound
Copy link
Contributor Author

TrySound commented May 1, 2018

@FezVrasta I meant with react. It's mixed interop. You should use either default one (which do not include types) or namespace (which will be used when react land esm).

@FezVrasta
Copy link
Member

Does the size-snapshot plugin create the minified bundle or do we have to add something like babili to generate it in a separated rollup config?

@TrySound
Copy link
Contributor Author

TrySound commented May 1, 2018

@FezVrasta Nope it's only track the size. I will add min.

rollup.config.js Outdated
@@ -1,8 +1,16 @@
import nodeResolve from 'rollup-plugin-node-resolve';
import commonjs from 'rollup-plugin-commonjs';
import babel from 'rollup-plugin-babel';
import uglify from 'rollup-plugin-uglify';
Copy link
Member

Choose a reason for hiding this comment

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

May we use rollup-plugin-babel-minify instead? I got better outputs with it in Popper.js

Copy link
Contributor Author

Choose a reason for hiding this comment

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

uglify 37.75 kb
babel-minify 38.15 kb

Copy link
Member

@FezVrasta FezVrasta May 1, 2018

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did it. Nothing is changed.

Copy link
Member

Choose a reason for hiding this comment

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

Cool thanks!


const input = './src/index.js';

const umdGlobals = {
Copy link
Member

Choose a reason for hiding this comment

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

Popper.js should be marked as external as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does it matter? Do you think somebody will use popper.js and react-popper in one script? For react-popper consumer there will be another action to add popper dependency.

Copy link
Member

Choose a reason for hiding this comment

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

I don't want to get every week a new issue with someone that asks me to release a new UMD bundle with the new Popper.js version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay.

rollup.config.js Outdated
{
input,
output: {
file: 'dist/index.min.js',
Copy link
Member

Choose a reason for hiding this comment

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

This should probably be index.umd.min.js to keep consistency

@FezVrasta
Copy link
Member

Should we add the UMD build to the browser entry of the package.json?

@TrySound
Copy link
Contributor Author

TrySound commented May 1, 2018

@FezVrasta Nope. It's bad. webpack prefers browser field over module.

@FezVrasta FezVrasta merged commit 114499f into floating-ui:master May 1, 2018
@TrySound TrySound deleted the bundle-umd branch May 1, 2018 11:46
@virgofx
Copy link

virgofx commented May 1, 2018

@TrySound your changes are inconsistent with v0.10.3+ can you update the rollup config to be similar. This way migrating to 1.0.0 will be consistent/easy.

Actually, I'll submit an updated PR. we have to fix the package.json in v10.3 so let me do that then also update in 1.0.x

@TrySound
Copy link
Contributor Author

TrySound commented May 1, 2018

This is a question of your config. I don't think this package should be used other than with from "react-popper" entry point.

@FezVrasta
Copy link
Member

@virgofx what do you mean exactly?

@virgofx
Copy link

virgofx commented May 1, 2018

It looks like the babelrcs/rollups aren't consistent. Just would prefer to make those consistent. I was going to take v0.10.x and incorporate back into v1; however Try beat me however file paths output different, it's missing the remove-prop-types fixes, and production best-practice with NODE_ENV as all done in v0.10.x

v.10.x

import babel from 'rollup-plugin-babel';
import minify from 'rollup-plugin-babel-minify';
import replace from 'rollup-plugin-replace';
import nodeResolve from 'rollup-plugin-node-resolve';

const baseConfig = (outputFormat) => {
  const isProduction = process.env.NODE_ENV === 'production';

  let file;
  switch (outputFormat) {
    case 'umd':
    case 'cjs':
      file = 'dist/' + outputFormat + '/react-popper' + (isProduction ? '.min' : '') + '.js';
      break;

    default:
      throw new Error('Unsupported output format: ' + outputFormat);
  }

  return {
    input: 'src/index.js',
    plugins: [
      nodeResolve(),
      babel({
        plugins: [
          // Ensure "external-helpers" is only included in rollup builds
          // Issue: https://github.com/rollup/rollup/issues/1595
          'external-helpers',
        ],
      }),
      replace({
        'process.env.NODE_ENV': JSON.stringify('production')
      }),
      isProduction ? minify({
        comments: false,
      }) : false,
    ],
    external: ['react', 'react-dom', 'prop-types', 'popper.js'],
    output: {
      name: 'ReactPopper',
      file: file,
      format: outputFormat,
      sourcemap: true,
      globals: {
        react: 'React',
        'prop-types': 'PropTypes',
        'popper.js': 'Popper',
      },
    },
  };
};

export default [
  baseConfig('cjs'),
  baseConfig('umd'),
];

1.0

import nodeResolve from 'rollup-plugin-node-resolve';
import commonjs from 'rollup-plugin-commonjs';
import babel from 'rollup-plugin-babel';
import uglify from 'rollup-plugin-uglify';
import { sizeSnapshot } from 'rollup-plugin-size-snapshot';

const input = './src/index.js';

const umdGlobals = {
  react: 'React',
  'react-dom': 'ReactDOM',
  'popper.js': 'Popper',
};

const getBabelOptions = () => ({
  babelrc: false,
  exclude: '**/node_modules/**',
  presets: [['env', { modules: false }], 'stage-1', 'react'],
  plugins: ['external-helpers'],
});

export default [
  {
    input,
    output: {
      file: 'dist/index.umd.js',
      format: 'umd',
      name: 'ReactPopper',
      globals: umdGlobals,
    },
    external: Object.keys(umdGlobals),
    plugins: [
      nodeResolve(),
      commonjs({ include: '**/node_modules/**' }),
      babel(getBabelOptions()),
      sizeSnapshot(),
    ],
  },

  {
    input,
    output: {
      file: 'dist/index.umd.min.js',
      format: 'umd',
      name: 'ReactPopper',
      globals: umdGlobals,
    },
    external: Object.keys(umdGlobals),
    plugins: [
      nodeResolve(),
      commonjs({ include: '**/node_modules/**' }),
      babel(getBabelOptions()),
      uglify(),
    ],
  },
];

Plus update corresponding package.json scripts, main, and 'umd:main`

I think v0.10.x is good now, we can cut v0.10.4 and that should be 100% ? If so, i'll make the corresponding changes now in 1.0

@FezVrasta
Copy link
Member

Note that in 1.x we are going to ship only UMD bundles, no CJS anymore

@virgofx
Copy link

virgofx commented May 1, 2018

That's totally fine, would prefer to drop the 'umd' sub folder then? or keep consistent? Plus was also going to incorporate readme updates

@FezVrasta
Copy link
Member

Umh, maybe it's a good idea to keep it in case in the future we decide to add more bundles (maybe once ES modules get widespread in the browsers?). Good call.

@virgofx
Copy link

virgofx commented May 1, 2018

Yeah, it doesn't hurt. I'll make it all consistent later today. Just cut v0.10.4 -- Let's make sure that's 100%. Then I'll submit a PR later for 1.0 with the same testing and fixes for the SSR/ production builds , readme updates, package.json updates, and rollup sync so everything is smooth upgrading.

@TrySound
Copy link
Contributor Author

TrySound commented May 1, 2018

I don't think there's anything missing in my config.

@TrySound
Copy link
Contributor Author

TrySound commented May 1, 2018

Probably replace for warning. Don't know what it is.

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

Successfully merging this pull request may close these issues.

None yet

3 participants