feat: update default npm export to ES5 js files #3245

moog16 · 2018-07-26T23:21:45Z

Currently npm is setup to export /index.js by default. The index.js file is ES6 and less universally accepted by other developers. This PR is to support those developers that are on other versions of EcmaScript, by exporting by default the dist js file.

Refs #929

See also material-components/material-components-web-react#107

BREAKING CHANGE: Anyone intending to build MDC Web's ES2015+ sources must directly import @material/foo/index. @material/foo will now resolve to UMD modules.

moog16 · 2018-07-26T23:23:56Z

packages/mdc-tab/package.json

@@ -9,7 +9,6 @@
    "material design",
    "tab"
  ],
-  "main": "index.js",


removed this because this isn't a package. Developer should be installing mdc-tabs

This is incorrect. mdc-tab is part of the new tabs work being done in the feat/tabs/tabs branch. Users will be installing this as well as mdc-tab-bar, mdc-tab-scroller, and mdc-tab-indicator.

Ah gotcha...I saw the export {MDCTabFoundation, MDCTab} from './tab'; line in mdc-tabs, and thought it to be this mdc-tab package. Just missing a . to become ../tab

This was never updated after my comment. Also, mdc-tab-bar, mdc-tab-scroller, and mdc-tab-indicator were added since this.

mdc-web-bot · 2018-07-26T23:32:55Z

All 128 screenshot tests passed for commit b55f5cc vs. master! 💯🎉

codecov-io · 2018-07-26T23:33:08Z

Codecov Report

Merging #3245 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #3245   +/-   ##
=======================================
  Coverage   98.46%   98.46%           
=======================================
  Files         120      120           
  Lines        5212     5212           
  Branches      654      654           
=======================================
  Hits         5132     5132           
  Misses         80       80

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5978499...3cbdb21. Read the comment docs.

mdc-web-bot · 2018-07-26T23:39:03Z

All 128 screenshot tests passed for commit e43c556 vs. master! 💯🎉

kfranqueiro

I would rather hold off on merging this until we can try and test a few things...

For instance, does this actually break every README we have which states import {MDCFoo} from '@material/foo'; because main no longer points to the main ES6 module?

moog16 · 2018-07-31T21:39:15Z

I think we'll also need to do a similar change as this PR: material-components/material-components-web-react#198

kfranqueiro · 2018-07-31T21:45:09Z

That's precisely the kind of thing I'm concerned about. I think that corroborates my concern about needing to update any ES2015 examples in our READMEs, because they would need to explicitly point to /index now.

Incidentally, our modules themselves should already be referencing /index where necessary, since #2111 was a thing. Otherwise we'd potentially have problems even in the module code itself.

moog16 · 2018-07-31T21:54:20Z

Would we want to point to the /index or the /dist/mdc.foo.js files? I believe we would want to point to /dist/mdc.foo, since pointing to /index wouldn't fix the issue.

Currently our readmes don't point to /index now, so why would they need to now? I think what #1991 is referring to is that Webpack/node will default to whatever the package.json main's prop is set to.

kfranqueiro · 2018-07-31T22:15:06Z

Would we want to point to the /index or the /dist/mdc.foo.js files? I believe we would want to point to /dist/mdc.foo, since pointing to /index wouldn't fix the issue.

Currently our readmes don't point to /index now, so why would they need to now? I think what #1991 is referring to is that Webpack/node will default to whatever the package.json main's prop is set to.

Do transpilers honor package.json's main property when resolving imports the way that node itself does? If they do, then all of our documented ES2015 usages would break, because they're all simply referencing the directory, not a specific file, and what they reference would change when we change main. This would also presumably break a large percentage of any end-user code that consumes our ES2015 modules.

moog16 · 2018-08-16T18:12:15Z

I wrote up a blog post of my learnings of the benefits of having npm packages export ES5 instead of ES6.

https://medium.com/@moog16/rules-of-npm-packaging-16a40c5d7e30

kfranqueiro

This is out of date with master and it looks like there are potentially some merge conflicts that are going to happen, based on what github is saying.

Also note that we've added more packages since this was opened (e.g. the new mdc-tab* packages, mdc-switch JS, and mdc-menu-surface).

Also, I don't know why this wasn't reported in CI, but I see a lot of test failures when I run unit tests locally. I also see incorrect coverage reports (and coverage well below the required 95% mark). Some of the coverage reports are now pointing to dist. I assume this is due to needing tests to explicitly reference mdc-foo/index after these changes, similar to material-components/material-components-web-react#198. (It concerns me that apparently the MDC React repo went over an entire month without correct coverage... we can't do that with MDC Web.)

I'm also still wanting to understand how our documented ES6 syntax still happens to work even when the entry point references the UMD files. I'd also like to know whether our getting started guide is impacted by this change at all.

kfranqueiro · 2018-08-22T13:54:01Z

packages/mdc-tab/package.json

@@ -9,7 +9,6 @@
    "material design",
    "tab"
  ],
-  "main": "index.js",


This was never updated after my comment. Also, mdc-tab-bar, mdc-tab-scroller, and mdc-tab-indicator were added since this.

kfranqueiro · 2018-08-22T13:54:09Z

packages/mdc-top-app-bar/package.json

@@ -3,6 +3,7 @@
  "version": "0.37.1",
  "description": "The Material Components for the web top app bar component",
  "license": "Apache-2.0",
+  "main": "mdc.topAppBar.js",


Missing dist?

mdc-web-bot · 2018-08-22T17:00:40Z

All 353 screenshot tests passed for commit 21955dd vs. master! 💯🎉

kfranqueiro · 2018-08-22T17:02:50Z

packages/mdc-form-field/package.json

@@ -8,7 +8,7 @@
    "material design",
    "form"
  ],
-  "main": "index.js",
+  "main": "mdc.formField.js",


Missing dist

kfranqueiro · 2018-08-22T17:03:58Z

packages/mdc-linear-progress/package.json

@@ -3,6 +3,7 @@
  "description": "The Material Components for the web linear progress indicator component",
  "version": "0.38.0",
  "license": "MIT",
+  "main": "dist/linearProgress.js",


Missing mdc.

mdc-web-bot · 2018-08-22T20:08:31Z

All 353 screenshot tests passed for commit cc1ee56 vs. master! 💯🎉

abhiomkar

Since we're pointing all packages to compiled version, can we have module property which points to ES6 for ES6 aware tool like webpack?

Reference: Rollup.js - Publishing ES6 modules

mdc-web-bot · 2018-08-22T20:44:46Z

All 353 screenshot tests passed for commit 39f9f31 vs. master! 💯🎉

moog16 · 2018-08-22T20:52:42Z

Pushed changes and performed following:

Linux:
npm run test:unit (passes)
npm run start (chrome)
npm run dev (chrome)

Windows:
npm run test:unit (passes)
npm run start (ie11 & edge)
npm run dev (ie11 & edge)

npm run start is not working on this branch or master branch on Windows 10 - I don't think it is an issue, since there are some PWD and $ in the start script. So all is good (regarding this PR)

mdc-web-bot · 2018-08-22T21:32:57Z

All 353 screenshot tests passed for commit 2e58cd5 vs. master! 💯🎉

mdc-web-bot · 2018-09-27T16:38:25Z

All 581 screenshot tests passed for commit 853f2d5 vs. master! 💯🎉

moog16 · 2018-09-27T16:57:17Z

docs/importing-js.md

+```
+
+Note that in this case, you must ensure your build toolchain is configured to process and transpile MDC Web's modules
+as well as your own. You will also need to include babel's `transform-object-assign` plugin for IE 11 support.


This piece of info should also be in the getting-started.md

https://github.com/material-components/material-components-web/blame/master/docs/getting-started.md#L222

Yeah, I had intended to look into the right place to add that to getting-started, thanks for reminding me.

I've fixed that in #3665, since that PR was already touching the lines around that point in the file. That way hopefully it'll merge cleanly the next time we sync this PR.

moog16 · 2018-09-27T17:05:00Z

scripts/verify-pkg-main.js

@@ -0,0 +1,66 @@
+/**


awesome script! Tested it and works nicely

mdc-web-bot · 2018-09-27T22:56:34Z

All 581 screenshot tests passed for commit ecacefc vs. master! 💯🎉

mdc-web-bot · 2018-09-28T13:22:27Z

All 581 screenshot tests passed for commit b1c6e83 vs. master! 💯🎉

kfranqueiro · 2018-09-28T13:56:03Z

Matt performed some tests with various build tools (e.g. browserify and webpack), and I tested this against our catalog and getting started guide. Between that and getting positive feedback from James on RMWC and Dominic on angular-mdc-web, I think we can merge this.

mdc-web-bot · 2018-09-28T14:17:06Z

All 581 screenshot tests passed for commit 44dde56 vs. master! 💯🎉

mdc-web-bot · 2018-09-28T17:39:31Z

All 581 screenshot tests passed for commit 7fcf8f1 vs. master! 💯🎉

mdc-web-bot · 2018-09-28T18:01:32Z

All 581 screenshot tests passed for commit 3cbdb21 vs. master! 💯🎉

…ents-web#3245

Matt Goo added 4 commits July 26, 2018 16:06

WIP: add main to package jsons

80eca3b

fix: added main props to package.jsons for remaining components

b55f5cc

Update package.json

6410afe

Update package.json

e43c556

moog16 commented Jul 26, 2018

View reviewed changes

kfranqueiro suggested changes Jul 27, 2018

View reviewed changes

kfranqueiro mentioned this pull request Jul 27, 2018

feat(switch): Merge updated switch into master #3214

Merged

moog16 mentioned this pull request Jul 27, 2018

Revert MDC Web imports to /dist material-components/material-components-web-react#183

Closed

kfranqueiro suggested changes Aug 22, 2018

View reviewed changes

Merge branch 'master' into chore/infra/update-default-package-es5

21955dd

kfranqueiro suggested changes Aug 22, 2018

View reviewed changes

fix: unit test

cc1ee56

abhiomkar reviewed Aug 22, 2018

View reviewed changes

fix: add main fields to packagejson

39f9f31

Merge branch 'master' into chore/infra/update-default-package-es5

2e58cd5

Merge branch 'master' into chore/infra/update-default-package-es5

94fbfda

googlebot added the cla: yes label Aug 28, 2018

kfranqueiro added this to the R20 milestone Sep 27, 2018

kfranqueiro changed the title ~~chore(infrastructure): update default npm export to ES5 js files~~ feat: update default npm export to ES5 js files Sep 27, 2018

WIP doc updates

853f2d5

moog16 commented Sep 27, 2018

View reviewed changes

scripts/verify-pkg-main.js

@@ -0,0 +1,66 @@

/**

Copy link

Contributor Author

moog16 Sep 27, 2018

Choose a reason for hiding this comment

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

awesome script! Tested it and works nicely

kfranqueiro mentioned this pull request Sep 27, 2018

docs: Pin babel versions in getting-started and improve formatting #3665

Merged

WIP Add link to babel plugin

ecacefc

kfranqueiro mentioned this pull request Sep 27, 2018

chore: Explicitly reference index.js under MDC packages material-components/material-components-web-catalog#211

Merged

WIP Update main README

b1c6e83

kfranqueiro approved these changes Sep 28, 2018

View reviewed changes

kfranqueiro and others added 2 commits September 28, 2018 09:56

Merge branch 'master' into chore/infra/update-default-package-es5

5149579

WIP Remove empty files introduced during an earlier merge

44dde56

Merge branch 'master' into chore/infra/update-default-package-es5

7fcf8f1

Merge branch 'master' into chore/infra/update-default-package-es5

3cbdb21

moog16 merged commit 514f9f8 into master Sep 28, 2018

moog16 deleted the chore/infra/update-default-package-es5 branch September 28, 2018 18:03

kfranqueiro mentioned this pull request Oct 3, 2018

Main inside package.json is referencing to es6 modules #929

Closed

jakemarsden added a commit to jakemarsden/aikido-track that referenced this pull request Oct 12, 2018

Update MDC imports to comply with material-components/material-compon…

7a2c184

…ents-web#3245

trimox mentioned this pull request Oct 19, 2018

Update imports to point directly to @material/foo/index trimox/angular-mdc-web#1462

Closed

jamesmfriedman mentioned this pull request Oct 30, 2018

Update to MDC 41 rmwc/rmwc#354

Closed

27 tasks

kfranqueiro mentioned this pull request Oct 30, 2018

Broken ES imports: declare ES module entry points #4016

Closed

4cm4k1 mentioned this pull request Nov 21, 2018

Bug: Compiled dist/mdc.drawer.js file drops ternary expression from tabbable package, giving SSRed apps "ReferenceError: Element is not defined" #4104

Closed

kfranqueiro mentioned this pull request Jan 3, 2019

Bundling Proposal #4222

Closed

feat: update default npm export to ES5 js files #3245

feat: update default npm export to ES5 js files #3245

Conversation

moog16 commented Jul 26, 2018 • edited by kfranqueiro Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdc-web-bot commented Jul 26, 2018

codecov-io commented Jul 26, 2018 • edited Loading

Codecov Report

mdc-web-bot commented Jul 26, 2018

kfranqueiro left a comment

Choose a reason for hiding this comment

moog16 commented Jul 31, 2018

kfranqueiro commented Jul 31, 2018

moog16 commented Jul 31, 2018

kfranqueiro commented Jul 31, 2018 • edited Loading

moog16 commented Aug 16, 2018

kfranqueiro left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdc-web-bot commented Aug 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdc-web-bot commented Aug 22, 2018

abhiomkar left a comment

Choose a reason for hiding this comment

mdc-web-bot commented Aug 22, 2018

moog16 commented Aug 22, 2018

mdc-web-bot commented Aug 22, 2018

mdc-web-bot commented Sep 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdc-web-bot commented Sep 27, 2018

mdc-web-bot commented Sep 28, 2018

kfranqueiro commented Sep 28, 2018

mdc-web-bot commented Sep 28, 2018

mdc-web-bot commented Sep 28, 2018

mdc-web-bot commented Sep 28, 2018

moog16 commented Jul 26, 2018 •

edited by kfranqueiro

Loading

codecov-io commented Jul 26, 2018 •

edited

Loading

kfranqueiro commented Jul 31, 2018 •

edited

Loading

kfranqueiro left a comment •

edited

Loading