chore(package): add `browser` field that points to UMD build #3566

soederpop · 2019-04-12T06:00:14Z

uses expose-loader to do this
added the umd build path as the browser property on package.json

welcome · 2019-04-12T06:00:17Z

💖 Thanks for opening this pull request! 💖

Here is a list of things that will help get it across the finish line:

Run yarn lint locally to catch formatting errors. This will fix some errors automatically, commit and push any changes.
Run yarn test locally to catch errors. This ensures all components still behave as they should.
Run yarn start to run the doc site locally and try a few pages, ensuring everything is in good working order.
Include tests when adding/changing behavior.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

codecov-io · 2019-04-12T06:03:59Z

Codecov Report

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

@@           Coverage Diff           @@
##           master    #3566   +/-   ##
=======================================
  Coverage   99.81%   99.81%           
=======================================
  Files         174      174           
  Lines        2730     2730           
=======================================
  Hits         2725     2725           
  Misses          5        5

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 6c93cdd...ba25c4f. Read the comment docs.

layershifter · 2019-04-12T08:03:25Z

#3565 (comment)

UMD build was already fixed, however thank you for your effort there ❤️
I also want to avoid add browser field, for me it's not clear how Webpack and Rollup will handle them...

soederpop · 2019-04-12T15:22:07Z

Please reconsider.

For webpack you have to opt in with config.

{ resolve: { aliasFields: ['browser'] } }

here are the docs on how it is used https://github.com/defunctzombie/package-browser-field-spec

Rollup also lets you opt into this:

https://github.com/rollup/rollup-plugin-node-resolve

So making this change won't effect anyone who uses these bundlers, except in the case that the developers make this very common config enhancement (or their boilerplate does) and then semantic-ui-react isn't able to help them optimize

but the biggest benefit would be for tools like unpkg.com and other cdn's, who would know to link to the umd build by default. instead they redirect to the commonjs build which is obviously not usable in the browser.

so it doesn't hurt anyone, and only helps the people who want it.

Also, the UMD build is actually broken, despite the test saying otherwise

<html>
  <head>
      <script src="https://unpkg.com/react@16.8.4/umd/react.production.min.js"></script>
      <script src="https://unpkg.com/react-dom@16.8.4/umd/react-dom.production.min.js"></script>
      <script src="https://unpkg.com/semantic-ui-react@0.86.0/dist/umd/semantic-ui-react.min.js"></script>
      <script>
        alert(typeof window.semanticUIReact === 'undefined' ? 'Oh Crap' : 'Hmm')
      </script>
  </head>
  <body>
      <h1>hi</h1>
  </body>
</html>

see the result:

tested on all browsers.

used the file from the npm package, from unpkg.com, and built locally -- same error

also, take a look at the UMD build's contents:

/*! no static exports found */
/*! all exports used */
/*! ModuleConcatenation bailout: Module is not an ECMAScript module */function(t,n){t.exports=e},
/*!***************************************************************!*\
  !*** ./node_modules/@babel/runtime/helpers/defineProperty.js ***!
  \***************************************************************/

30% or more of the bundle is this kind of stuff, even though TerserPlugin is configured with comments set to false in the output.

maybe the test isn't testing the same build you're actually distributing.

soederpop · 2019-04-12T15:26:07Z

Also I confirmed this bug did not exist until 0.85.0. 80-84 are all fine.

I took the liberty of modifying your UMD test to include the UMD builds that have been published and the tests do fail starting at 0.85 and 0.86, but pass at 0.84

I do think there may be an issue between what your tests are actually testing, vs what you're distributing.

layershifter · 2019-04-12T15:43:18Z

Fix was not published yet, that's why it it's still broken. You can manually build master and get working UMD. I got your point, let's revert changes with expose-loader and merge.

soederpop · 2019-04-12T15:55:46Z

Will do!

~~I don't think the version I built from master without expose-loader was working but maybe I rushed through it, will test it out again.~~

Sorry to bombard you I didn't realize that the master branch was ahead of the npm release even though the version numbers are the same.

soederpop · 2019-04-12T16:49:43Z

thanks a lot for reconsidering. removed the expose-loader. confirmed it works fine. apologies for the false alarm.

welcome · 2019-04-15T10:15:20Z

Congrats on merging your first pull request! 🎉🎉🎉

Reverts changes introduced in #3566, fixes #3597.

* chore(package): remove `browser` field Reverts changes introduced in #3566, fixes #3597. * Update package.json

…c-Org#3566)

* chore(package): remove `browser` field Reverts changes introduced in Semantic-Org#3566, fixes Semantic-Org#3597. * Update package.json

layershifter closed this Apr 12, 2019

layershifter reopened this Apr 12, 2019

Add browser property to package.json

ba25c4f

soederpop force-pushed the master branch from cfe4371 to ba25c4f Compare April 12, 2019 15:57

layershifter changed the title ~~fix(build) UMD build exposes semanticUIReact~~ chore(package): add browser field that points to UMD build Apr 15, 2019

layershifter merged commit c532f72 into Semantic-Org:master Apr 15, 2019

layershifter mentioned this pull request May 6, 2019

Module not found: Can't resolve 'React' in '/srv/node_modules/semantic-ui-react/dist/umd' #3597

Closed

layershifter added a commit that referenced this pull request May 6, 2019

chore(package): remove browser field

563ee11

Reverts changes introduced in #3566, fixes #3597.

layershifter mentioned this pull request May 6, 2019

chore(package): remove browser field #3598

Merged

layershifter added a commit that referenced this pull request May 6, 2019

chore(package): remove browser field (#3598)

97c4ab9

* chore(package): remove `browser` field Reverts changes introduced in #3566, fixes #3597. * Update package.json

mbakiev pushed a commit to mbakiev/Semantic-UI-React that referenced this pull request Jun 17, 2019

chore(package): add browser field that points to UMD build (Semanti…

f5f0bae

…c-Org#3566)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(package): add `browser` field that points to UMD build #3566

chore(package): add `browser` field that points to UMD build #3566

soederpop commented Apr 12, 2019

welcome bot commented Apr 12, 2019

codecov-io commented Apr 12, 2019 •

edited

Loading

layershifter commented Apr 12, 2019

soederpop commented Apr 12, 2019

soederpop commented Apr 12, 2019 •

edited

Loading

layershifter commented Apr 12, 2019 •

edited

Loading

soederpop commented Apr 12, 2019 •

edited

Loading

soederpop commented Apr 12, 2019

welcome bot commented Apr 15, 2019

chore(package): add browser field that points to UMD build #3566

chore(package): add browser field that points to UMD build #3566

Conversation

soederpop commented Apr 12, 2019

welcome bot commented Apr 12, 2019

codecov-io commented Apr 12, 2019 • edited Loading

Codecov Report

layershifter commented Apr 12, 2019

soederpop commented Apr 12, 2019

soederpop commented Apr 12, 2019 • edited Loading

layershifter commented Apr 12, 2019 • edited Loading

soederpop commented Apr 12, 2019 • edited Loading

soederpop commented Apr 12, 2019

welcome bot commented Apr 15, 2019

chore(package): add `browser` field that points to UMD build #3566

chore(package): add `browser` field that points to UMD build #3566

codecov-io commented Apr 12, 2019 •

edited

Loading

soederpop commented Apr 12, 2019 •

edited

Loading

layershifter commented Apr 12, 2019 •

edited

Loading

soederpop commented Apr 12, 2019 •

edited

Loading