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

[core] Change UMD output name to 'MaterialUI' #13142

Merged
merged 2 commits into from
Feb 1, 2019

Conversation

tkrotoff
Copy link
Contributor

@tkrotoff tkrotoff commented Oct 8, 2018

Change UMD output name from 'material-ui' to 'MaterialUI'.
This is a breaking change.

Closes #12387

See also Stack Overflow question: How do I add Material-UI to a JSFiddle project

Breaking change

This change is easing the usage of Material-UI with a CDN:

const {
  Button,
  TextField,
-} = window['material-ui'];
+} = MaterialUI;

It's consistent with the other projects:

  • material-ui => MaterialUI
  • react-dom => ReactDOM
  • prop-types => PropTypes

You need to update the CDN example once this change is released.

@oliviertassinari oliviertassinari added this to the v4 milestone Oct 8, 2018
@oliviertassinari
Copy link
Member

Have we considered the MUI wording? cc @mui-org/core-contributors

@oliviertassinari oliviertassinari added on hold There is a blocker, we need to wait breaking change labels Oct 8, 2018
@tkrotoff
Copy link
Contributor Author

tkrotoff commented Oct 8, 2018

  • react-dom => "ReactDOM"
  • prop-types => "PropTypes"

Makes sense to follow the same pattern:

  • material-ui => "MaterialUI"

@@ -7,7 +7,6 @@ import { uglify } from 'rollup-plugin-uglify';
import { sizeSnapshot } from 'rollup-plugin-size-snapshot';

const input = './src/index.js';
const name = 'material-ui';
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please change only this line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I keep and change name to be 'MaterialUI' then the file name will be changed aswell: build/umd/MaterialUI.production.min.js.
Probably not what we want, dashes are more common and better reflects the project name/typo: build/umd/material-ui.production.min.js.

Copy link
Contributor

@TrySound TrySound Oct 8, 2018

Choose a reason for hiding this comment

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

Okay, hardcode file names but reuse the name.

Copy link
Contributor Author

@tkrotoff tkrotoff Oct 8, 2018

Choose a reason for hiding this comment

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

I didn't do it on purpose because it does not add any value.

If you variabilize name then the file name and format should be treated equally:

const baseFileName = 'build/umd/material-ui';
const format = 'umd';
const outputName = 'MaterialUI';
// ...
output: {
  file: `${baseFileName}.development.js`,
  format,
  name: outputName,
  globals
}

vs

// ...
output: {
  file: 'build/umd/material-ui.development.js',
  format: 'umd',
  name: 'MaterialUI',
  globals
}

I believe the later form is more readable (the file is only 60 lines long and will stay small over time).
Sometimes variabilize/factorize small things can make them less readable.



We can try to be a smart asses and reduce lines of code by 2:

const getConfig = mode => ({
  input,
  output: {
    file: `build/umd/material-ui.${mode}.js`,
    format: 'umd',
    name: 'MaterialUI',
    globals
  },
  external: Object.keys(globals),
  plugins: [
    nodeResolve(),
    babel(babelOptions),
    commonjs(commonjsOptions),
    nodeGlobals(),
    replace({ 'process.env.NODE_ENV': JSON.stringify(mode) }),
    mode === 'production' && sizeSnapshot(),
    mode === 'production' && uglify()
  ]
});

export default [
  getConfig('development'),
  getConfig('production')
];

well, the added value on a very small file like this one is 0 and just make things harder IMHO.

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean hard code file name in rollup config. Without such tricky overconfiguration. And reuse the name of global variable.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

const input = './src/index.js';
const name = 'MaterialUI';
const globals = {
  react: 'React',
  'react-dom': 'ReactDOM',
};
const babelOptions = {
  exclude: /node_modules/,
  // We are using @babel/plugin-transform-runtime
  runtimeHelpers: true,
  configFile: '../../babel.config.js',
};
const commonjsOptions = {
  ignoreGlobal: true,
  include: /node_modules/,
};

export default [
  {
    input,
    output: {
      file: 'build/umd/material-ui.development.js',
      format: 'umd',
      name,
      globals,
    },
    external: Object.keys(globals),
    plugins: [
      nodeResolve(),
      babel(babelOptions),
      commonjs(commonjsOptions),
      nodeGlobals(),
      replace({ 'process.env.NODE_ENV': JSON.stringify('development') }),
    ],
  },
  {
    input,
    output: {
      file: 'build/umd/material-ui.production.min.js',
      format: 'umd',
      name,
      globals,
    },
    external: Object.keys(globals),
    plugins: [
      nodeResolve(),
      babel(babelOptions),
      commonjs(commonjsOptions),
      nodeGlobals(),
      replace({ 'process.env.NODE_ENV': JSON.stringify('production') }),
      sizeSnapshot(),
      uglify(),
    ],
  },
];

@eps1lon eps1lon removed the on hold There is a blocker, we need to wait label Feb 1, 2019
@eps1lon eps1lon changed the base branch from master to next February 1, 2019 06:42
@eps1lon eps1lon mentioned this pull request Feb 1, 2019
56 tasks
@oliviertassinari oliviertassinari merged commit 8531269 into mui:next Feb 1, 2019
@oliviertassinari
Copy link
Member

@tkrotoff Thank you!

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.

4 participants