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

Move to new ember-fetch import path #732

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

asakusuma
Copy link

@asakusuma asakusuma commented Dec 20, 2022

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.

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

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
Copy link
Contributor

ef4 commented Dec 20, 2022

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.

README.md Outdated Show resolved Hide resolved
@@ -0,0 +1,13 @@
/*
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a blocker for this PR, but we should also think about how to deprecate this.

@chriskrycho
Copy link
Contributor

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
Copy link
Author

@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
Copy link
Author

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
Copy link
Contributor

ef4 commented Dec 22, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants