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

Provide a way to easily mock fetch and Web platform polyfills #295

Merged
merged 11 commits into from Apr 12, 2019

Conversation

@maier49
Copy link
Contributor

@maier49 maier49 commented Mar 18, 2019

Type: feature

The following has been addressed in the PR:

Description:

Partially addresses #275

@maier49 maier49 requested a review from matt-gadd Mar 18, 2019
}

export function replace(fetch: (input: RequestInfo, init?: RequestInit) => Promise<Response>) {
if (has('test')) {
Copy link
Contributor

@samends samends Mar 18, 2019

Choose a reason for hiding this comment

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

What is has referring to here?

Choose a reason for hiding this comment

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

Copy link
Contributor

@samends samends Mar 18, 2019

Choose a reason for hiding this comment

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

thanks!

package.json Outdated
@@ -50,11 +50,11 @@
"globalize": "1.4.0",
"immutable": "3.8.2",
"intersection-observer": "0.4.2",
"isomorphic-fetch": "^2.2.1",
Copy link
Contributor

@dasa dasa Mar 18, 2019

Choose a reason for hiding this comment

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

cross-fetch might be a better option for this (I haven't used either): https://github.com/lquixada/cross-fetch#why-not-isomorphic-fetch

Copy link
Contributor Author

@maier49 maier49 Mar 23, 2019

Choose a reason for hiding this comment

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

Good point, I've updated this to use cross-fetch

@maier49 maier49 changed the title Provide a way to easily mock fetch Provide a way to easily mock fetch and Web platform polyfills Mar 23, 2019
package.json Outdated
@@ -45,6 +45,7 @@
"@types/globalize": "0.0.34",
"@webcomponents/webcomponentsjs": "1.1.0",
"cldrjs": "0.5.0",
"cross-fetch": "^3.0.1",
Copy link
Member

@agubler agubler Mar 30, 2019

Choose a reason for hiding this comment

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

Can we fix this dep please?


export default global.fetch.bind(global) as (input: RequestInfo, init?: RequestInit) => Promise<Response>;
let fetchLoadedPromise: Promise<void>;
if (!has('build-elide')) {
Copy link
Member

@agubler agubler Mar 30, 2019

Choose a reason for hiding this comment

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

I think this causes issues with the bundling on the build side - we get two unnamed bundles (one for the browser and one for node).

Although when building in legacy mode it looks like it's actually bundling correctly in the platform/fetch bundle.

Copy link
Contributor Author

@maier49 maier49 Apr 8, 2019

Choose a reason for hiding this comment

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

I'm not sure what the best way to resolve this would be then. Seems like what we'd really want is two separate static imports, one with
!has('build-elide') as well as has('host-node') and one for the browser.

But I'm not sure that would behave as expected.

Choose a reason for hiding this comment

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

so I think we don't actually ever need to run fetch in node as node is only a supported target environment under test. given thats the complaint (whatwg-fetch blowing up in tests), we can perhaps just use a polyfill that doesn't, and then we don't need to complicate this. For example: https://github.com/developit/unfetch I think doesn't blow up.

@@ -116,5 +117,45 @@ export interface AnimationConstructor {
new (effect?: AnimationEffectReadOnly, timeline?: AnimationTimeline): Animation;
}

export const Animation = global.Animation as AnimationConstructor;
export const KeyframeEffect = global.KeyframeEffect as KeyframeEffectConstructor;
const _Animation = global.Animation as AnimationConstructor;
Copy link
Member

@agubler agubler Mar 30, 2019

Choose a reason for hiding this comment

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

Do you think it's possible to wrap all the mocking / replacement parts in a has check for test? At build time this should be then converted to false and then removed from the prod build?

Copy link
Contributor Author

@maier49 maier49 Apr 8, 2019

Choose a reason for hiding this comment

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

👍 I'll just need to make a change in cli-build-app then to set this flag for the test build.

const _intersectionObserver: typeof IntersectionObserver = global.IntersectionObserver as typeof IntersectionObserver;
let replacement: typeof IntersectionObserver = _intersectionObserver;

const Wrapper = class {

Choose a reason for hiding this comment

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

This seems a bit verbose to have to do for each shim, also I don't think we need to wrap this outside of a test. I wonder if we could just essentially take your wrapping constructor idea and use it generically across the modules, kind of like a "live binding" wrapper to the global? That way we wouldn't even need a replace function per shim, they could just change the global?

function wrapper(nameOnGlobal: string) {
    return function(...args: any[]) {
        return new global[nameOnGlobal](...args);
    };
}

What IntersectionObserver.ts would then look like:

import global from './global';
`!has('build-elide')`;
import 'intersection-observer';
import has from '../has/has';
import wrapper from './somewhere/wrapper';

let value = global.IntersectionObserver;

if (has('test')) {
    value = wrapper('IntersectionObserver') ;
}

export default value as typeof IntersectionObserver;


export default global.fetch.bind(global) as (input: RequestInfo, init?: RequestInit) => Promise<Response>;
let fetchLoadedPromise: Promise<void>;
if (!has('build-elide')) {

Choose a reason for hiding this comment

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

so I think we don't actually ever need to run fetch in node as node is only a supported target environment under test. given thats the complaint (whatwg-fetch blowing up in tests), we can perhaps just use a polyfill that doesn't, and then we don't need to complicate this. For example: https://github.com/developit/unfetch I think doesn't blow up.


export default global.IntersectionObserver as typeof IntersectionObserver;
export default wrapper('IntersectionObserver', true) as typeof IntersectionObserver;
Copy link
Member

@agubler agubler Apr 9, 2019

Choose a reason for hiding this comment

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

Could we only use the wrapper when the has flag for test is true? This means that when in dist or dev the native implementation will be used (and also prevents the user from being able to change the implementation in anything other than test).

Copy link
Contributor Author

@maier49 maier49 Apr 9, 2019

Choose a reason for hiding this comment

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

I have the wrapper now returning the original when the test flag is not set to true. So they'll always get the native/polyfilled implementation.

As in, it returns global[nameOfThing] one time instead of a function that provides a live binding.

}
}

return global[nameOnGlobal].bind(global);

Choose a reason for hiding this comment

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

if we can avoid doing this outside of test that would be ideal, just because otherwise say window.IntersectionObserver is not going to have equality with this return due to the wrapping bind?

Copy link
Contributor Author

@maier49 maier49 Apr 9, 2019

Choose a reason for hiding this comment

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

The only reason I put this here is because fetch was already doing this. I could just put another parameter on wrapper to control this and make sure it only happens for fetch, or remote that entirely if it wasn't necessary in the first place.

Choose a reason for hiding this comment

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

@maier49 cool, perhaps it wasn't needed in the first place?

Copy link
Member

@agubler agubler Apr 9, 2019

Choose a reason for hiding this comment

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

The bind on fetch was added here: #192

Copy link
Contributor Author

@maier49 maier49 Apr 9, 2019

Choose a reason for hiding this comment

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

@matt-gadd I can't think of or find any reason why it would be. I've gone ahead and removed it.

Copy link
Member

@agubler agubler Apr 9, 2019

Choose a reason for hiding this comment

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

@maier49 did you see the original issue as to why fetch was bound?

Copy link
Contributor Author

@maier49 maier49 Apr 9, 2019

Choose a reason for hiding this comment

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

@agubler Nope, sorry, missed that. I will add another flag to the wrapper then.

Copy link
Contributor Author

@maier49 maier49 Apr 9, 2019

Choose a reason for hiding this comment

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

Fixed

@maier49 maier49 requested a review from matt-gadd Apr 11, 2019
@maier49 maier49 merged commit b77638b into dojo:master Apr 12, 2019
4 checks passed
@maier49 maier49 deleted the 275-nice-shim-mocks branch Apr 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants