ember-cli / ember-fetch

HTML5 fetch polyfill from github wrapped and bundled for ember-cli users.
MIT License
176 stars 82 forks source link

Move to new ember-fetch import path #732

Open asakusuma opened 1 year ago

asakusuma commented 1 year ago

This will ensure there is no confusion around folks thinking they are importing from the actual fetch package.

import fetch from 'fetch' > import fetch from 'ember-fetch'

For backwards compatibility, we need to still support consumers importing from the fetch path. In the future, we should release a breaking change which removes this alias and requires consumers to import from ember-fetch.

ef4 commented 1 year ago

The goal here is good. But the implementation can get a lot simpler, because most of what it was doing before was only needed in order to escape its own module namespace.

For example, emitting the implementation in treeForVendor is not needed. It can just be normal files in the ./addon folder.

chriskrycho commented 1 year ago

Ah, perfect – I was going to ask about exactly that, @ef4. Net, @asakusuma we can leave the existing nonsense in place for now, but have the new implementation just… be a normal implementation.

asakusuma commented 1 year ago

@ef4 @chriskrycho thanks. naive question, but wouldn't adding an implementation in the addon folder duplicate the implementation code twice in every build? I'm assuming there's a reason the original implementation didn't start with the code in the addon folder and then just re-export to fetch via the treeFor stuff.

asakusuma commented 1 year ago

From what I can tell, there are some additional things going on in treeForVendor beyond escaping the namespace, which requires build time logic and injecting code from node_modules. I don't see a simple way to keep this logic and external package injection and move the code into the addon folder:

https://github.com/ember-cli/ember-fetch/blob/master/index.js#L172-L175

In other words, I don't doubt there's a way to move some of the code into the addon folder, but I'm not seeing an approach that does that without increasing the overall complexity.

ef4 commented 1 year ago

All that resolving NPM packages and rolling them up is stuff that ember-auto-import natively does for you.

This add only needs to emit import statements within treeForAddon, based on the options.