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

Builds a UMD file for the browser when publishing. #8

Merged
merged 1 commit into from
May 2, 2016

Conversation

qubyte
Copy link
Owner

@qubyte qubyte commented May 1, 2016

This PR coaxes browserify into making a standalone build. This allows the built file to be used in AMD environments as well as a browser global.

@paulmelnikow This is an alternative to #5.

@paulmelnikow
Copy link

paulmelnikow commented May 1, 2016

Neat approach!

I rebased my tests on this version and updated them to require the pre-built file. The webpack build works correctly but generates this warning:

WARNING in ./build/fetch-browser.js
Critical dependencies:
1:486-493 This seems to be a pre-built javascript file. Though this is possible, it's not recommended. Try to require the original source to get better results.
 @ ./build/fetch-browser.js 1:486-493

The browserify build works fine, and makes no complaint.

A slight disadvantage to this approach is that whatwg-fetch gets pulled in as a string literal and can't be minified.

To try to dodge this warning, do you think it would be worth shipping the CommonJS and UMD builds separately?

Here's the branch with the tests: https://github.com/paulmelnikow/fetch-ponyfill/tree/test_umd_version

"brfs": "1.4.0",
"browserify": "13.0.0",
"mkdirp": "0.5.1",
"node-fetch": "1.5.1",

Choose a reason for hiding this comment

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

For require('fetch-ponyfill')() to work from Node, node-fetch needs to stay in dependencies.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Whoops, good catch.

@qubyte
Copy link
Owner Author

qubyte commented May 1, 2016

I have an idea on how to make this leaner. It means ditching browserify and making a hand rolled builder. I'd rather not separate UMD stuff where possible, but hand rolling can make it a lot smaller. I'll experiment...

@qubyte qubyte force-pushed the feature/umd-browser branch 2 times, most recently from ef86afa to daa2566 Compare May 1, 2016 19:08
@qubyte
Copy link
Owner Author

qubyte commented May 1, 2016

@paulmelnikow Take a look now. There's still a bit of module fluff in there, but the rest is minifier friendly. It's pretty hacktastic, but then so was the original.

@qubyte
Copy link
Owner Author

qubyte commented May 1, 2016

It also fixes an issue of the existing version not working in a web worker without manually passing in config.

@paulmelnikow
Copy link

Builds clean, and minifies! I like it. 👍

When you publish, it looks like fetch-browser.js and build.js will be included, while build/fetch-browser.js will not.

You can either use .npmignore or the files argument. I think the second is probably cleaner:

"files": [
  "fetch-node.js",
  "build/fetch-browser.js"
],

That seems to be the right incantation. You can use npm pack or pkgfiles to test what you're getting.

@qubyte
Copy link
Owner Author

qubyte commented May 1, 2016

Odd. That should have worked. In any case, including the files list is a good idea (less stuff in the tar ball). Nice catch! I'll update.

@qubyte
Copy link
Owner Author

qubyte commented May 1, 2016

Added and checked, seems to be working.

@paulmelnikow
Copy link

Looks good!

@qubyte qubyte merged commit fd732a5 into master May 2, 2016
@qubyte qubyte deleted the feature/umd-browser branch May 2, 2016 10:10
@paulmelnikow paulmelnikow mentioned this pull request May 2, 2016
@paulmelnikow
Copy link

Stoked!

Would you mind doing an npm release so I can try to integrate it?

@qubyte
Copy link
Owner Author

qubyte commented May 2, 2016

No problem. I'll bump the whatwg-fetch version and release it.

@qubyte
Copy link
Owner Author

qubyte commented May 2, 2016

Released as v1.0.0 (tracking whatwg version).

@paulmelnikow
Copy link

We started integrating this today, and it's working great!

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.

2 participants