search

karlvr / openapi-generator-plus-generators

Other
21 stars 8 forks source link

Exclude explicit fetch polyfill import #19

Closed psamusev closed 3 years ago

psamusev commented 3 years ago

Allow through configuration exclude the import of the whatwg-fetch. I know that it's possible through custom templates but more safe option is do it through configuration flag

Changes for package @openapi-generator-plus/typescript-fetch-client-generator

karlvr commented 3 years ago

@psamusev thanks for reporting this issue. I just moved to using whatwg-fetch as I was frustrated with the missing types on portable-fetch. I created the @openapi-generator-plus/typescript-fetch-node-generator to workaround this, as that uses node-fetch explicitly. I'm considering using fetch-ponyfill instead, if I fix the missing RequestInit type there... which shouldn't be hard.

But... what's kept me away from it is that we do require some more third party modules in node to support us. It seems that the URLSearchParams is often provided by including the DOM library, or just built into node somehow? But we also use FormData if there's any multipart requests. So in @openapi-generator-plus/typescript-fetch-node-generator I'm importing the form-data package to provide that functionality... I'm not sure whether that package works as a polyfill.

So basically... it's a little complicated. I'm about to face this myself as I have a project that shares the API library between web and React Native.

What do you think about the above?

psamusev commented 3 years ago

What I've understood you use whatwg-fetch just to be able use this code in IE 11 for instance, which doesn't have this API. however for modern browser including of this polypill is just extra bundle size and useless library installation. What I've intended to describe is to make the possibility either include `import 'whatwg-fetch' to result generated file or exclude with help of configuration property.

So, basically the issue is about flexible way of including/excluding piece of code that's fully optional for modern browsers. And this issue specifically for @openapi-generator-plus/typescript-fetch-client-generator package

psamusev commented 3 years ago

Regarding your problem, there is polyfill for URLSearchParams as well as you've already use form-data for FormData on node. So for node part both of these polyfill are required where in browser we have build-in solution for modern

karlvr commented 3 years ago

@psamusev thank you for this clarification. Yes, you make good points... I'll check out the polyfill situation for FormData and see where we get to. It does make sense for the end-user to include a fetch polyfill if they want to support those browsers.

karlvr commented 3 years ago

@psamusev I have investigated making a single module with main and browser and react-native keys in the package.json... but I think it would require some rearchitecting as the generators for each of those are slightly different. I've deferred thinking about that and instead added an includePolyfills options for @openapi-generator-plus/typescript-fetch-client-generator so you can disable the use of polyfills in the output! Maybe in future I'll change the polyfills used to be more node compatible, but I think it's better—as you noted—to keep the build size small for browsers.

psamusev commented 3 years ago

@karlvr just tested the latest version of @openapi-generator-plus/typescript-fetch-client-generator, it still contains import in typescript code. I see you added check to package dependencies https://github.com/karlvr/openapi-generator-plus-generators/blob/master/packages/typescript-fetch-client/templates/hooks/packageDependencies.hbs, however you've missed it https://github.com/karlvr/openapi-generator-plus-generators/blob/master/packages/typescript-fetch-client/templates/hooks/runtimeImports.hbs

karlvr commented 3 years ago

@psamusev amateur hour obviously! I've pushed a fix and will do a new release this morning.