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

Not recommended: require-fetch-import #1224

Open
ef4 opened this issue Jun 4, 2021 · 5 comments
Open

Not recommended: require-fetch-import #1224

ef4 opened this issue Jun 4, 2021 · 5 comments

Comments

@ef4
Copy link
Contributor

ef4 commented Jun 4, 2021

I see that there is a plan to add require-fetch-import to the recommended rule set.

I don't think this is a good idea. It pushes people toward a pattern that we are already trying to kill: importing from paths that aren't real packages.

At a minimum, I don't think we should enable this by default until ember-cli/ember-fetch#330 is addressed.

Personally, I don't think it's appropriate to require people to import fetch at all. It's a web standard, linting against it in people's code is contrary to our efforts to bring ember into better alignment with javascript-ecosystem-wide standards, and the global fetch is equally easy to patch/polyfill in all environments (like tests and fastboot) where it's helpful to do that.

@bmish
Copy link
Member

bmish commented Jun 4, 2021

@ef4 sounds good, I will remove that from the plan. Would you like to edit the require-fetch-import rule doc to explain these caveats? That will help make it more clear why some may choose not to use this rule.

@bmish
Copy link
Member

bmish commented Jun 4, 2021

CC: @Turbo87

@Turbo87
Copy link
Member

Turbo87 commented Jun 5, 2021

FWIW this pushes us towards a vastly different way of writing testing code and if we want move into such a direction where test waiters are no longer a thing it should require an RFC

@ef4
Copy link
Contributor Author

ef4 commented Jun 5, 2021

FWIW this pushes us towards a vastly different way of writing testing code and if we want move into such a direction where test waiters are no longer a thing it should require an RFC

You're misunderstanding me. We can still hook into fetch the exact same way for tests. That has nothing to do with making people type import fetch from "fetch".

An unbound variable named fetch and an import from "fetch" are equally easy to detect and replace with whatever implementation people want, including test waiters. The difference is that one reads as normal web-standards javascript and one requires teaching people why ember is being weird.

Also, if we take control of global fetch, then your test waiters will work even when the fetch is in an NPM package that's not ember-specific.

@Turbo87
Copy link
Member

Turbo87 commented Jun 5, 2021

okay, that sounds reasonable. the rule was built to address issues with the current reality where the global fetch does not integrate with test waiters. if we have plans to improve that then that's fine with me, but until that is the case I would still consider this rule quite useful to avoid flaky tests due to missing test waiters.

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 a pull request may close this issue.

3 participants