Do not include polyfill if browser targets don't need it #173

mydea · 2018-11-27T15:19:52Z

This is my stab at fixing #45.

If the target browsers all support native fetch, this will avoid importing the polyfill(s) and just setup the exports, so that you can continue using import fetch from 'fetch'; with no code changes.

By default, it will use your specified target browsers, but you can also override this in the options, e.g.:

'ember-fetch': {
  preferNative: true,
  browers: ['ie 11']
}

If no browsers are found at all, it will always include the polyfill.

This PR is on top of #172 , as I could not get to the configuration via this.buildConfig in the treeForBrowserFetch method.

If you have any suggestions for improvements, I'd be happy to try to implement them - To be honest, it was a bit tricky to implement this, so there might be better ways to do some things.

As a sidenote, I wasted some time until I figured out that the polyfill is included in development environment even if it shouldn't (due to the browser targets), as ember-cli-pretender will also include the polyfill in development/test environment. So as long as you have ember-cli-pretender installed, you'll need to actually run ember s --prod to get a build without the polyfill (if you remove IE 11 from the targets for production).

xg-wang · 2018-11-27T23:19:04Z

Why do we need the browsers option if we already have target.js?

mydea · 2018-11-28T08:03:26Z

I basically lifted this from ember-cli-autoprefixer - it also makes testing a bit easier. In theory, it could also be used for special needed behavior, e.g. forcing including of the polyfill even though your targets don't need it. But I can remove the option if so desired :)

mydea · 2018-12-10T14:02:45Z

So, do you want me to remove the browsers option, or should I leave it in?

xg-wang

Github had an issue if you don't leave a comment when requesting changes all inline comments would lost 🤕

index.js

xg-wang · 2018-12-11T07:09:53Z

assets/browser-fetch.js.t

@@ -33,6 +33,10 @@

    <%= moduleBody %>

+    if (!self.fetch) {


This error should be changed to provide more actionable info if we agree on changes below.

xg-wang · 2018-12-11T07:32:17Z

index.js

@@ -113,7 +116,8 @@ module.exports = {
    }

    const app = this._findApp();
-    const preferNative = app._fetchBuildConfig.preferNative;
+    // If the polyfill is not included, we force the preferNative behavior
+    const preferNative = app._fetchBuildConfig.preferNative || this._checkSupports('fetch');


I think we should respect preferNative over browser support and alwaysIncludePolyfill. The reason is people may be relying some xhr features and don't want native fetch even it exists.

But if we do not include the polyfill, we need to set preferNative to true - otherwise, it will fail.
If we have two options: preferNative (default false) and alwaysIncludePolyfill (default true).

Possible combinations:

{ preferNative: true, alwaysIncludePolyfill: true } --> polyfill is always included, but might not be used.

{ alwaysIncludePolyfill: false } --> In this case, the polyfill might be loaded or not loaded (it might either load fetch & abortcontroller polyfills, or only the abortcontroller polyfill, or none). If at least one polyfill is NOT loaded, then preferNative has to be true, otherwise the unpolyfilled parts will not work.

I was saying the logic should be inverse: if the preferNative is set to true, then check second option alwaysIncludePolyfill and browser supports.

The behavior would be

{ preferNative: true, alwaysIncludePolyfill: true } --> polyfills are always included, but might not be used

{ preferNative: true, alwaysIncludePolyfill: false } --> conditionally include polyfills based on browser support

{ preferNative: false } --> skip checking alwaysIncludePolyfill and caniuse API request, always include polyfills

Ah, got it.
I guess the question is, does it make sense for alwaysIncludePolyfill to default to true then, really? I mean, I guess it is more of an edge case to say: "I want to use the native polyfill when possible, but still always include the polyfill file"? Because now, to get this (IMHO most sensible) outcome, you need to set:

'ember-fetch': { preferNative: true, alwaysIncludePolyfill: false }

Which feels a bit counter-intuitive - as these two options are pretty entangled and depend on each other in difficult to understand ways (e.g. setting alwaysIncludePolyfill does nothing if preferNative=false, ...). So I guess, wouldn't it be easier to understand to make alwaysIncludePolyfill default to false, so you can still, if needed for whatever edge-case reason, opt out of it, but the "happy path" is much simpler by just setting preferNative: true?

Yea agree you're right about the ergonomics here. Previously I thought caniuse-api was making HTTP requests but after some digging I realized they are packing the dataset with the package. So the result of preferNative: true should be only the enhancement of reducing the redundant asset.
To reflect the opt-out path for edge-cases you mentioned, can you also update the README about the new API?

xg-wang · 2018-12-11T07:37:25Z

index.js

@@ -131,6 +135,8 @@ module.exports = {
  // wrapped in a shim that stops it from exporting a global and instead turns it into a module
  // that can be used by the Ember app.
  treeForBrowserFetch() {
+    const needsFetchPolyfill = !this._checkSupports('fetch');


The issue here is we don't just polyfill fetch, we polyfill AbortController as well. And they have different browser support. caniuse also has a note

Safari has window.AbortController defined in the DOM but it's just a stub, it does not abort requests at all. The same issue also affects Chrome on IOS and Firefox on IOS because they use the same WebKit rendering engine as Safari.

which is detected by our polyfill lib programmatically. I'm only brain debugging this... so apparent we need to verify API response or we come up with a better way.

But I totally agree if preferNative && supportNativeFetchAndAbortController && !alwaysIncludePolyfill we should just strip off the polyfill.

Or if apart from preferNative and alwaysIncludePolyfill we can provide 2 separate group of these for fetch and abortController with reasonable defaults. User can have finer-grained control of this addon. What do you think?

True, I did not think about this - Can I Use has just recently updated this to correctly state that abortcontroller does not work in Safari. When I looked the last time, it seemed as if all fetch-supporting browsers also supported abortcontroller - which is clearly not the case.

We can just load the necessary polyfills - e.g. fetch & abortcontroller, or only abortcontroller, or none!

mydea · 2018-12-11T09:43:23Z

So I've pushed another commit, checking AbortController support separately from fetch support.
This includes a new alwaysIncludePolyfill option, which defaults to true (e.g. the previous behavior - which might be changed in an upcoming major version I guess?). If this is true, it will always include the fetch and the AbortController polyfills.

If this is set to false, it will check if all browser targets support fetch and/or AbortController, and include only the necessary polyfills based on browser support. If at least one of the polyfills is not included, it will automatically set preferNative to true to ensure the non-polyfilled code works as expected.

xg-wang

left few comments

index.js

E.g. on Safari, fetch is supported but abortcontroller is not, so we need to load the polyfills conditionally there.

If `preferNative=false`, it will now never check for fetch support and always include the polyfill.

xg-wang

few comments

index.js

test/browsers-target-test.js

index.js

Also make `alwaysIncludePolyfill` default to false.

mydea · 2018-12-18T08:09:14Z

I pushed another set of changes, addressing the review. alwaysIncludePolyfill now defaults to false, and I've added a paragraph to the readme explaining the new polyfill semantics. I've also moved the option-reading up in the treeForVendor and pass the options to the treeForBrowserFetch method as argument. I also revised the test messages (I've just copied them from the preferNative tests and forgot to change them)

xg-wang · 2018-12-19T02:54:42Z

README.md

@@ -96,6 +96,8 @@ otherwise the polyfilled `fetch` will be installed during the first pass of the

 If set to `false`, the polyfilled `fetch` will replace native `fetch` be there or not.

+If all your browser targets (defined in `config/targets.js`) support native `fetch`, and `preferNative: true`, the polyfill will not be included in the output build. If, for some reason, you still need the polyfill to be included in the bundle, you can set `alwaysIncludePolyfill: true`.


Suggested change

If all your browser targets (defined in `config/targets.js`) support native `fetch`, and `preferNative: true`, the polyfill will not be included in the output build. If, for some reason, you still need the polyfill to be included in the bundle, you can set `alwaysIncludePolyfill: true`.

If all your [browser targets](https://guides.emberjs.com/release/configuring-ember/build-targets/) support native `fetch`, and `preferNative: true`, the polyfill will not be included in the output build. If, for some reason, you still need the polyfill to be included in the bundle, you can set `alwaysIncludePolyfill: true`.

I think the first paragraph

If set to true, the the fetch polyfill will be skipped if native fetch is available, ...

can just be replaced by your added paragraph.

Or let me know what do you think would be best, we should be good now to release a version 🍻

Hmm, but that would then not cover the case that preferNative: true, but some targets do not have fetch support (which, for now, will probably be the case for many apps) - in that case, the setting will still have an effect (--> not use the polyfill if fetch is available).

Co-Authored-By: mydea <francesco@cropster.com>

mydea changed the title ~~Use browserlist~~ Do not include polyfill if browser targets don't need it Nov 27, 2018

mydea mentioned this pull request Nov 28, 2018

Allow opt-out of fetch polyfill rwjblue/ember-cli-pretender#67

Merged

mydea force-pushed the use-browserlist branch from 41dcc28 to f831bf6 Compare December 3, 2018 08:29

xg-wang requested changes Dec 11, 2018

View reviewed changes

mydea force-pushed the use-browserlist branch from f831bf6 to ccacbc0 Compare December 11, 2018 09:39

xg-wang requested changes Dec 16, 2018

View reviewed changes

index.js Outdated Show resolved Hide resolved

index.js Outdated Show resolved Hide resolved

index.js Outdated Show resolved Hide resolved

Francesco Novy added 3 commits December 17, 2018 11:43

Do not include fetch polyfill if target browsers don't need it

2c4e636

Ensure abortcontroller polyfill is included if it is not supported

27a87fc

E.g. on Safari, fetch is supported but abortcontroller is not, so we need to load the polyfills conditionally there.

Require preferNative=true to check for fetch support

0df78a3

If `preferNative=false`, it will now never check for fetch support and always include the polyfill.

mydea force-pushed the use-browserlist branch from e9227bf to 0df78a3 Compare December 17, 2018 10:43

xg-wang requested changes Dec 18, 2018

View reviewed changes

Streamline fetch browser target checks & tests

2e7bfa6

Also make `alwaysIncludePolyfill` default to false.

xg-wang reviewed Dec 19, 2018

View reviewed changes

xg-wang added the enhancement label Dec 19, 2018

xg-wang approved these changes Dec 19, 2018

View reviewed changes

Update README.md

d326603

Co-Authored-By: mydea <francesco@cropster.com>

xg-wang merged commit 029cc6f into ember-cli:master Dec 19, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not include polyfill if browser targets don't need it #173

Do not include polyfill if browser targets don't need it #173

mydea commented Nov 27, 2018

xg-wang commented Nov 27, 2018

mydea commented Nov 28, 2018

mydea commented Dec 10, 2018

xg-wang left a comment

xg-wang Dec 11, 2018

xg-wang Dec 11, 2018

mydea Dec 11, 2018

xg-wang Dec 16, 2018

mydea Dec 17, 2018

xg-wang Dec 18, 2018

xg-wang Dec 11, 2018 •

edited

Loading

mydea Dec 11, 2018

mydea commented Dec 11, 2018

xg-wang left a comment

xg-wang left a comment

mydea commented Dec 18, 2018

xg-wang Dec 19, 2018 •

edited

Loading

mydea Dec 19, 2018

		@@ -96,6 +96,8 @@ otherwise the polyfilled `fetch` will be installed during the first pass of the

		If set to `false`, the polyfilled `fetch` will replace native `fetch` be there or not.

		If all your browser targets (defined in `config/targets.js`) support native `fetch`, and `preferNative: true`, the polyfill will not be included in the output build. If, for some reason, you still need the polyfill to be included in the bundle, you can set `alwaysIncludePolyfill: true`.

	If all your browser targets (defined in `config/targets.js`) support native `fetch`, and `preferNative: true`, the polyfill will not be included in the output build. If, for some reason, you still need the polyfill to be included in the bundle, you can set `alwaysIncludePolyfill: true`.
	If all your [browser targets](https://guides.emberjs.com/release/configuring-ember/build-targets/) support native `fetch`, and `preferNative: true`, the polyfill will not be included in the output build. If, for some reason, you still need the polyfill to be included in the bundle, you can set `alwaysIncludePolyfill: true`.

Do not include polyfill if browser targets don't need it #173

Do not include polyfill if browser targets don't need it #173

Conversation

mydea commented Nov 27, 2018

xg-wang commented Nov 27, 2018

mydea commented Nov 28, 2018

mydea commented Dec 10, 2018

xg-wang 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xg-wang Dec 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mydea commented Dec 11, 2018

xg-wang left a comment

Choose a reason for hiding this comment

xg-wang left a comment

Choose a reason for hiding this comment

mydea commented Dec 18, 2018

xg-wang Dec 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xg-wang Dec 11, 2018 •

edited

Loading

xg-wang Dec 19, 2018 •

edited

Loading