add UMD on package #545
add UMD on package #545
Conversation
|
I don't really understand why some of those tests fails. I don't think I'm responsible for this. |
|
Any input on this? |
There was a problem hiding this comment.
- You've broken CI for webworker environment. More information in comments.
- We're not yet sure whether we want to support the UMD construct, but I appreciate you suggesting this. Let us get back to you on our decision regarding modularization of the fetch package.
- The diff is larger than it should be. Try to reduce the diff until there is the minimum amount of changes required for the UMD shim to work.
- If we merged your change and pulled the updated package into GitHub.com where we use it, it would have broken GitHub.com. We load all compatible modules via AMD system, and because your change makes this library skip the polyfill part when it's loaded via AMD,
window.fetchwould never be declared this way, and the site would be broken for older browsers. You need to consider the fact that there has to be a method of enabling the polyfill even if the module is loaded through AMD or CommonJS.
| @@ -463,4 +489,6 @@ | |||
| }) | |||
| } | |||
| self.fetch.polyfill = true | |||
| })(typeof self !== 'undefined' ? self : this); | |||
There was a problem hiding this comment.
You've removed this construct which was here to make fetch work for both normal browser and webworker environments. You've replaced it with two different contexts: this for the concept of the "root" in the UMD shim, and window || global for the concept of "main" inside the factory function. This is likely what broke the test suite for webworker.
I think the concepts of "root" and "main" should be unified and should be passed around as a single reference. Also, per #521, I think the fallback chain for getting a reference to the global object should be (pseudocode) self || this || global.
| "URLSearchParams": false, | ||
| "define": true, | ||
| "module": true, | ||
| "global": true |
There was a problem hiding this comment.
Isn't there a jshint "environment" declaration akin to "worker": true that makes it assume that these globals are available; possibly "umd": true or "amd": true?
There was a problem hiding this comment.
Sadly, I didn't find any. There are a lot of things like that in eslint but I cannot find those for jshint.
fetch.js
Outdated
| } | ||
|
|
||
| var support = { | ||
| searchParams: 'URLSearchParams' in self, |
There was a problem hiding this comment.
This unnecessarily increases the size of the diff. Try naming the global object self instead of main. Also, please pass the global object by argument into the factory function instead of determining it inside the factory.
There was a problem hiding this comment.
I cannot name it self because I already defined it later to avoid diffs on more important stuff (since what was self is now split between self = what is exported and main = global object). I'll rename it root while doing the other changes you suggest so it keeps the same nomenclature as the UMD definition.
|
Seems like now all tests are green ! Thanks. I also moved the code that exported in global the polyfill to the main codebase so it happens for AMD & commonjs too just in case some people already loaded the file in such environments one way or another and expected it to auto export fetch. I don't really like it because I do believe it should not autoload the polyfill in such a use case but I don't want to break anyone. What are your thoughts about this? |
|
@qraynaud Thank you for your contribution, and sorry that this didn't get merged. We weren't ready to add a UMD wrapper back then, but we were ready now, since a need arose to have module exports (specifically: abortable fetch requests). However, we've made a build step that generates a UMD build via Rollup. #616 |
I think this is a good alternative for both #521 and #180.
Adding and UMD allows the polyfill to be loaded in more environments and let the user choose between their native implementation and the polyfill if they want and whenever they want.