build/distribute as UMD #616

jayphelps · 2018-03-29T22:36:06Z

~~I still need to adjust tests--still trying to figure out if I can change things while still keeping the existing makefile setup, is that the preference?~~ Ready!

This was requested before #592 can be merged.

jayphelps · 2018-03-29T22:37:47Z

package.json

  },
  "files": [
    "LICENSE",
-    "fetch.js"
+    "dist/whatwg-fetch.js"


If it's desired we could build to ./fetch.js instead of putting things inside dist (and naming it something else). I did it this way as it's more traditional in the community, but otherwise your call.

jayphelps · 2018-03-29T22:38:11Z

webpack.config.js

+  output: {
+    filename: 'whatwg-fetch.js',
+    path: path.resolve(__dirname, 'dist'),
+    library: 'WHATWGFetch',


Wasn't sure what to call the global object. Any preferences?

jayphelps · 2018-03-29T22:52:41Z

src/index.js

@@ -0,0 +1,476 @@
+'use strict';


The way github is displaying this isn't great (it doesn't realize the file move/rename and changes). Here is the separate commit with whitespace ignored (?w=1 flag) a5ec273?w=1#diff-1fdf421c05c1140f6d71444ea2b27638

mislav · 2018-03-30T10:57:52Z

Thank you for working on this! What do you think about skipping webpack dependency altogether (it might be an overkill), and wrap current fetch.js with something like this:

(function(root, factory) {
    if (typeof define === 'function' && define.amd) {
        define([], function(){ return factory({}, root) })
    } else if (typeof module === 'object' && module.exports) {
        factory(module.exports, root)
    } else {
        factory({}, root)
    }
}(typeof self !== 'undefined' ? self : this, function(exports, global) {

    // current fetch.js contents

    if (!global.fetch) {
        global.fetch = fetch
        // ...
    }

    exports.fetch = fetch
    exports.Request = Request
    // ...
    return exports
}))

jayphelps · 2018-04-06T18:54:23Z

@mislav based on our PM conversations I've updated the PR to have the UMD preamble inline. I also added a simple test to verify the exports are there in CJS. ✌️

jayphelps · 2018-04-06T18:57:34Z

fetch.js

+(function(root, factory) {
+  /* jshint strict:false */
+  /* globals define, module */
+  if (typeof define === 'function' && define.amd) {


I initially tried to use/* jshint ignore: start */ (and end) but it didn't work in 2.8.0 jshint (maybe jshint/jshint#2411). After upgrading to jshint 2.9.0 it works, but then it says factory is unused, and everything I tried to ignore that caused even more issues, and 2.9.0 also detects other undefined variables like Promise/Symbol. So this seemed to be the solution with the least amount of changes.

mislav

Great start!

mislav · 2018-04-06T22:20:41Z

fetch.js

-    searchParams: 'URLSearchParams' in self,
-    iterable: 'Symbol' in self && 'iterator' in Symbol,
-    blob: 'FileReader' in self && 'Blob' in self && (function() {
+    searchParams: 'URLSearchParams' in global,


To keep the diff to a minimum, could global simply continue to be called self like before?

mislav · 2018-04-06T22:37:52Z

script/test

@@ -7,3 +7,5 @@ if [ -n "$SAUCE_BROWSER" ]; then
 else
  ./script/phantomjs
 fi
+
+./node_modules/mocha/bin/mocha test/cjs.js


Why is this separate file needed?

These tests could be in test.js. That file already jumps through hoops to exercise both native and polyfilled fetch, and this module approach could potentially clean it up.

A separate file was used as it is testing whether the CJS exports are available and the easiest and foolproof way seemed to be to do it inside a real CJS environment. test.js is evaluated in the context of a browser, where no require exists, and I did not see any other tests running inside a CJS env. Sorry about that, am I missing an easier solution? I could make an ajax request then eval it inside a fake CJS environment stub but this seemed harder and more brittle. lmk what you had in mind.

@mislav any thoughts? 🕺

@mislav hey there. Anything I can do to move this forward? I'm truly sorry to be that nagging person 😞I realize OSS is mostly volunteer and super appreciate what you've done for everyone! We're approaching a company deadline for getting in dependencies and we really need abort polyfill support for fetch so it's important for us to get this in so we can then get #592 in as well. I'm happy to do any leg work, just need to know what to do 😃

phil-lgr · 2018-04-13T21:26:32Z

Hey let us know if we can help here! This is a much needed feature!

graingert · 2018-04-27T15:00:41Z

fyi https://www.npmjs.com/package/fetch-ponyfill already provides a module for this

stefanpenner · 2018-05-14T18:02:21Z

@mislav anything I can do to help get this, or something similar landed? I would hate to have rely on a fork long-term.

First of all, loading our UMD dist file in Node doesn't work due to usage of `self` within `fetch.js`. Second, we don't need to test exports, since we trust Rollup doing its job.

Prior to this change, `test.js` had to be loaded before `fetch.js` so it can nulify the native implementation of `window.fetch` to force testing of the polyfill. Now that we have exports from the UMD file, we can abandon this hack and simply access the exports to juggle between native and polyfilled versions of the implementation.

Outdated

mislav · 2018-05-17T14:39:27Z

Hi @jayphelps, I've just pushed my proposed changes to this branch. Thank you for waiting.

Everything was OK with the state of the PR as you've left it. The static UMD wrapper looked good. However, on our side, some new requirements to how we package our open source libraries have crept in. Specifically, we want our packages to support a module keyword in addition to the main keyword in package.json. The module file would be an entrypoint that is an ES5 file with ES6 export syntax. This is compatible with build tools such as Rollup.

I have made a call to have fetch.js be a plain, unwrapped implementation of the polyfill with ES6 exports, therefore suitable to be pointed to by module. This would mean, however, that it would be best to have a build step that converts that into a UMD file, like you've first proposed in the initial version of this PR. Instead of Webpack, I use Rollup here because its what we use in our other open source projects (sometimes we use Babel to generate the UMD file if the project already uses Babel).

Because jshint was giving me trouble through this transition, I've switched to eslint & prettier like we also do in our other open source projects. This resulted in a lot of apparent code changes that are mostly just prettier adjusting whitespace. This makes the diff of this PR look huge, but I think the transition is worth it in the long run. The diff is best viewable while ignoring whitespace changes.

I would appreciate a review. Thank you! 🙏

stefanpenner · 2018-05-17T21:12:21Z

I've switched to eslint & prettier like we also

Good call!

koddsson

This looks good to me :)

jayphelps · 2018-05-22T21:42:45Z

LGTM

jayphelps · 2018-05-22T22:17:57Z

A somewhat related offshoot concern I still have is that the current proposed abort support in #592 won't polyfill window.fetch if window.fetch is already assigned but doesn't itself support aborting. My gut says that would be non-obvious to people and potentially cause an issue where aborting works as expected in one browser but doesn't in another. With this PR adding modules, is there potential to reconsider #592's behavior or alternatively provide a way to opt-in to it always polyfilling if something isn't 100% supported? e.g. it would override window.fetch if any only if it doesn't support abort signals. If that as the default behavior isn't acceptable, perhaps an opt-in module? import 'whatwg-fetch/polyfill-with-abort';

The approach we took in yetch was that the default import/export of the project didn't polyfill anything so it could be used as a library without side effects. If they wanted to instead polyfill you use import import 'yetch/polyfill';. My intent isn't to advertise yetch, just using it as a reference.

graingert · 2018-05-22T22:30:53Z

Ponyfills like yetch are always the preferred option

…

On Tue, 22 May 2018, 23:18 Jay Phelps, ***@***.***> wrote: A somewhat related offshoot concern I still have is that the current proposed abort support in #592 <#592> won't polyfill window.fetch if window.fetch is already assigned but doesn't itself support aborting. My gut says that would be non-obvious to people and potentially cause an issue where aborting works as expected in one browser but doesn't in another. With this PR adding modules, is there potential to reconsider #592 <#592 behavior or alternatively provide a way to opt-in to it always polyfilling if something isn't 100% supported? e.g. it would override window.fetch if any only if it doesn't support abort signals. If that as the default behavior isn't acceptable, perhaps an opt-in module? import 'whatwg-fetch/polyfill-with-abort'; The approach we took in yetch was that the default import/export of the project didn't polyfill anything so it could be used as a library without side effects. If they wanted to instead polyfill you use import import 'yetch/polyfill';. My intent isn't to advertise yetch, just using it as a reference. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#616 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAZQTMGVBhIssTR0Q-aiiSPbPibbNItdks5t1I6lgaJpZM4TBFB4> .

keithamus · 2018-05-23T01:26:42Z

@jayphelps I do not disagree - but I think this is a good first step. Let's wait for this patch to land and then look into those subsequent issues.

mislav · 2018-05-23T13:39:41Z

The approach we took in yetch was that the default import/export of the project didn't polyfill anything so it could be used as a library without side effects.

There's definitely value in the ponyfill approach, especially in the world where using modules is becoming more and more standard. However, you have to consider the existing whatwg-fetch user base which is very wide and far-reaching in terms of history. We've advertised this as a polyfill from the start and I would guess with relative confidence that a large slice of our user base is simply including fetch.js in their project and using window.fetch after that.

I'm not ready to pull the plug on this behavior and make the polyfill an opt-in just yet, even through a major version bump, especially because this library didn't provide exports in the past. So, my plan is to first provide the exports (while keeping the implicit polyfill functionality), have our users get used to that, and possibly switch to the ponyfill approach months down the line through another major version bump.

Thanks for your input!

Github/fetch has merged PR for UMD distribution: JakeChampion/fetch#616 Also abort support has been added: JakeChampion/fetch#592 We can use Github fetch instead of yetch now. But AbortableFetch is still not supported, see comment in test.

Adapted from JakeChampion/fetch#616

jayphelps commented Mar 29, 2018

View reviewed changes

jayphelps force-pushed the umd branch 2 times, most recently from 53ca60e to a5ec273 Compare March 29, 2018 22:49

jayphelps commented Mar 29, 2018

View reviewed changes

jayphelps force-pushed the umd branch 3 times, most recently from 18f8d4a to a05392c Compare April 6, 2018 18:52

jayphelps changed the title ~~WIP: build/distribute as UMD~~ build/distribute as UMD Apr 6, 2018

jayphelps force-pushed the umd branch from a05392c to 0de9490 Compare April 6, 2018 18:56

jayphelps commented Apr 6, 2018

View reviewed changes

mislav previously requested changes Apr 6, 2018

View reviewed changes

Distribute as UMD

da6e028

jayphelps force-pushed the umd branch from 0de9490 to da6e028 Compare April 9, 2018 17:37

This comment has been minimized.

Sign in to view

jayphelps mentioned this pull request May 7, 2018

Support abort API #592

Merged

3 tasks

mislav added 9 commits May 16, 2018 16:56

Unwrap fetch.js to be a clean module file

8aec47c

Add exports to fetch.js

9728193

Run prettier on source file

03d01c5

Make fetch.js compatible with eslint

24c3538

Switch from jshint to eslint

3ccb1ce

Have tests pass eslint

5d80368

Run test files through prettier

0a57487

Build UMD dist file with Rollup

432bf07

Quit testing CJS exports directly

114e2bc

First of all, loading our UMD dist file in Node doesn't work due to usage of `self` within `fetch.js`. Second, we don't need to test exports, since we trust Rollup doing its job.

mislav added 2 commits May 17, 2018 16:22

Merge remote-tracking branch 'origin/master' into umd

9595e3e

Try testing with a newer Node version

7d8326b

koddsson approved these changes May 18, 2018

View reviewed changes

keithamus approved these changes May 18, 2018

View reviewed changes

mislav merged commit f83e371 into JakeChampion:master May 23, 2018

This was referenced May 25, 2018

add UMD on package #545

Closed

Add additional fallbacks when self is undefined #521

Closed

MattiasBuelens added a commit to MattiasBuelens/yetch that referenced this pull request Jul 5, 2018

Switch to eslint

6c1cccf

Adapted from JakeChampion/fetch#616

github-actions bot locked as resolved and limited conversation to collaborators May 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build/distribute as UMD #616

build/distribute as UMD #616

jayphelps commented Mar 29, 2018 •

edited

jayphelps Mar 29, 2018

jayphelps Mar 29, 2018

jayphelps Mar 29, 2018

mislav commented Mar 30, 2018 •

edited

jayphelps commented Apr 6, 2018

jayphelps Apr 6, 2018

mislav left a comment

mislav Apr 6, 2018

jayphelps Apr 9, 2018

mislav Apr 6, 2018

jayphelps Apr 9, 2018 •

edited

jayphelps Apr 12, 2018

jayphelps Apr 16, 2018 •

edited

jayphelps May 7, 2018

phil-lgr commented Apr 13, 2018

graingert commented Apr 27, 2018

This comment has been minimized.

stefanpenner commented May 14, 2018

mislav commented May 17, 2018 •

edited

stefanpenner commented May 17, 2018

koddsson left a comment

jayphelps commented May 22, 2018

jayphelps commented May 22, 2018

graingert commented May 22, 2018 via email

keithamus commented May 23, 2018

mislav commented May 23, 2018

build/distribute as UMD #616

build/distribute as UMD #616

Conversation

jayphelps commented Mar 29, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mislav commented Mar 30, 2018 • edited

jayphelps commented Apr 6, 2018

Choose a reason for hiding this comment

mislav left a comment

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

jayphelps Apr 9, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jayphelps Apr 16, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phil-lgr commented Apr 13, 2018

graingert commented Apr 27, 2018

This comment has been minimized.

stefanpenner commented May 14, 2018

mislav commented May 17, 2018 • edited

stefanpenner commented May 17, 2018

koddsson left a comment

Choose a reason for hiding this comment

jayphelps commented May 22, 2018

jayphelps commented May 22, 2018

graingert commented May 22, 2018 via email

keithamus commented May 23, 2018

mislav commented May 23, 2018

jayphelps commented Mar 29, 2018 •

edited

mislav commented Mar 30, 2018 •

edited

jayphelps Apr 9, 2018 •

edited

jayphelps Apr 16, 2018 •

edited

mislav commented May 17, 2018 •

edited