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

build: set AMD module module names within UMD bundles #7233

Merged

Conversation

devversion
Copy link
Contributor

Currently if the UMD bundles from @material/<..> NPM modules are
loaded in the browser, using RequireJS or other loaders, the
AMD modules would need to be named manually using a loader
configuration. e.g.

require.config({
  paths: {
      '@material/animation': '/base/npm/node_modules/@material/animation/dist/mdc.animation',
      ...
  }
}

This could be avoided if the AMD define invocations in the UMD file
would specify a module id, similar to how it's done in packages of the
Angular organization. This allows for easier consumption in tests as well.

Note that this change causes a breaking change for the UMD-case where
the exports are bound to a global variable. Previously the entry-point
would appear in camel-case, but now it's matching the actual package
name in dash-case. This is unfortunately not avoidable with the current
Webpack tooling. i.e. previous UMD users relying on the globals (which are
rather rare anyway), would need to switch from
window.mdc.circularProgress to window.mdc['circular-progress].

@google-cla google-cla bot added the cla: yes label Jul 14, 2021
@devversion devversion force-pushed the build/umd-bundles-module-name branch from 3466720 to 7dad947 Compare July 14, 2021 18:48
@devversion devversion marked this pull request as ready for review July 14, 2021 18:53
@devversion devversion force-pushed the build/umd-bundles-module-name branch 4 times, most recently from c0e7353 to 2de7779 Compare July 15, 2021 19:02
@abhiomkar
Copy link
Contributor

Hi Paul -- can you add context of this change? why is this change required? Target audience who would benefit from this change?

Can we add this feature without breaking global variable changes? (i.e., window.mdc.circularProgress) by using existing global variable names as IDs?

@devversion
Copy link
Contributor Author

Hey @abhiomkar. I've tried providing some context with the snippet in the PR description but here is some more:

Basically, right now, MDC ships package output as UMD output. The UMD files contain logic for dealing with the AMD format. i.e.

	else if(typeof define === 'function' && define.amd)
		define([], factory);

The logic for AMD can be seen in the snippet above. One thing that is striking in the example above is that there is no AMD module name specified. A so-called anonymous module. This results in unnecessary complexity for consumers which intend to rely on AMD modules. This can be the case for a lot of people in the web ecosystem. i.e. people using Karma for unit tests, or development apps running with SystemJS or RequireJS etc. This also impacts the Angular organization where we use Karma w/ RequireJS for unit tests, and RequireJS for the development apps.

The problem with the so-called anonymous AMD modules is that consumers cannot simply start loading the UMD file from MDC, but rather need to setup a module loader configuration to map each @material<..> module name to the anonymous AMD module file. e.g. here is an example from Angular Components:

require.config({
  paths: {
    '@material/animation': '/base/npm/node_modules/@material/animation/dist/mdc.animation',
    '@material/auto-init': '/base/npm/node_modules/@material/auto-init/dist/mdc.autoInit',
    '@material/base': '/base/npm/node_modules/@material/base/dist/mdc.base',
    '@material/checkbox': '/base/npm/node_modules/@material/checkbox/dist/mdc.checkbox',
    '@material/chips': '/base/npm/node_modules/@material/chips/dist/mdc.chips',
    '@material/circular-progress': '/base/npm/node_modules/@material/circular-progress/dist/mdc.circularProgress',
    '@material/dialog': '/base/npm/node_modules/@material/dialog/dist/mdc.dialog',
    '@material/dom': '/base/npm/node_modules/@material/dom/dist/mdc.dom',
    '@material/drawer': '/base/npm/node_modules/@material/drawer/dist/mdc.drawer',
    '@material/floating-label': '/base/npm/node_modules/@material/floating-label/dist/mdc.floatingLabel',
    '@material/form-field': '/base/npm/node_modules/@material/form-field/dist/mdc.formField',
    '@material/grid-list': '/base/npm/node_modules/@material/grid-list/dist/mdc.gridList',
    '@material/icon-button': '/base/npm/node_modules/@material/icon-button/dist/mdc.iconButton',
    '@material/line-ripple': '/base/npm/node_modules/@material/line-ripple/dist/mdc.lineRipple',
    '@material/linear-progress': '/base/npm/node_modules/@material/linear-progress/dist/mdc.linearProgress',
    '@material/list': '/base/npm/node_modules/@material/list/dist/mdc.list',
    '@material/menu': '/base/npm/node_modules/@material/menu/dist/mdc.menu',
    '@material/menu-surface': '/base/npm/node_modules/@material/menu-surface/dist/mdc.menuSurface',
    '@material/notched-outline': '/base/npm/node_modules/@material/notched-outline/dist/mdc.notchedOutline',
    '@material/radio': '/base/npm/node_modules/@material/radio/dist/mdc.radio',
    '@material/ripple': '/base/npm/node_modules/@material/ripple/dist/mdc.ripple',
    '@material/select': '/base/npm/node_modules/@material/select/dist/mdc.select',
    '@material/slider': '/base/npm/node_modules/@material/slider/dist/mdc.slider',
    '@material/snackbar': '/base/npm/node_modules/@material/snackbar/dist/mdc.snackbar',
    '@material/switch': '/base/npm/node_modules/@material/switch/dist/mdc.switch',
    '@material/tab': '/base/npm/node_modules/@material/tab/dist/mdc.tab',
    '@material/tab-bar': '/base/npm/node_modules/@material/tab-bar/dist/mdc.tabBar',
    '@material/tab-indicator': '/base/npm/node_modules/@material/tab-indicator/dist/mdc.tabIndicator',
    '@material/tab-scroller': '/base/npm/node_modules/@material/tab-scroller/dist/mdc.tabScroller',
    '@material/data-table': '/base/npm/node_modules/@material/data-table/dist/mdc.dataTable',
    '@material/textfield': '/base/npm/node_modules/@material/textfield/dist/mdc.textfield',
    '@material/tooltip': '/base/npm/node_modules/@material/tooltip/dist/mdc.tooltip',
    '@material/top-app-bar': '/base/npm/node_modules/@material/top-app-bar/dist/mdc.topAppBar',
  }
});

if MDC would just provide a module name/id (as per the AMD spec), this would not be needed for consumers. They would just be able to load the UMD file, and the AMD module would be registered automatically with e.g. @material/textfield. Tests would then be able to just import from @material/textfield. This results in less maintenance, and easier consumption.

Here is an example of how the named AMD module should look like:

define("@material/textfield", [...], factory)

Re: the breaking change. Unfortunately it seems not avoidable because Webpack being used for library bundling. It does not support different module names between AMD, and "root". Either we'd not match the AMD module name with the NPM package, or we break the "root" global identifier. The latter seems more unlikely to be noticed.

Note that Angular also ships named AMD modules for that reason.

@abhiomkar
Copy link
Contributor

abhiomkar commented Jul 20, 2021

Thanks for detailed explanation!

Can you also update docs in docs/ directory? Some of them uses dot notation to access global variable (i.e, mdc.foo.FooBar, should be mdc['foo'].FooBar).

Currently if the UMD bundles from `@material/<..>` NPM modules are
loaded in the browser, using RequireJS or other loaders, the
AMD modules would need to be named manually using a loader
configuration. e.g.

```js
require.config({
  paths: {
      '@material/animation': '/base/npm/node_modules/@material/animation/dist/mdc.animation',
      ...
  }
}
```

This could be avoided if the AMD `define` invocations in the UMD file
would specify a module id, similar to how it's done in packages of the
Angular organization. This allows for easier consumption in tests as well.

Note that this change causes a breaking change for the UMD-case where
the exports are bound to a global variable. Previously the entry-point
would appear in camel-case, but now it's matching the actual package
name in dash-case. This is unfortunately not avoidable with the current
Webpack tooling. i.e. previous UMD users relying on the globals (which are
rather rare anyway), would need to switch from
`window.mdc.circularProgress` to `window.mdc['circular-progress]`.
@devversion devversion force-pushed the build/umd-bundles-module-name branch from 8d11f61 to eb9e221 Compare July 21, 2021 19:40
@devversion
Copy link
Contributor Author

@abhiomkar I've fixed one instance in the docs which has changed. For things like mdc.foo nothing has changed, and an actual property access is still possible. Should we keep that as is? (I think there is no benefit in using a keyed access here)

Copy link
Contributor

@abhiomkar abhiomkar left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @devversion!

@abhiomkar abhiomkar merged commit 9808de0 into material-components:master Jul 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants