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

Rebase tchack use fetch #5900

Merged
merged 4 commits into from
Mar 9, 2019

Conversation

NullVoxPopuli
Copy link
Sponsor Contributor

This just rebases @tchak's work from here: #5386

@runspired
Copy link
Contributor

failure is this._requireBuildPackages is not a function for why it won't build. There is also a lint error to address.

@dcyriller
Copy link
Contributor

NullVoxPopuli#1 (targeting your branch @NullVoxPopuli) fixes the yarn.lock issues and the lint error.

Also, you might be interested by this PR (also targeting your branch @NullVoxPopuli): NullVoxPopuli#2.

@NullVoxPopuli
Copy link
Sponsor Contributor Author

Both merged!

Copy link
Contributor

@runspired runspired left a comment

Choose a reason for hiding this comment

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

Hopefully this is the very last review, I gave this a last look through and noticed that in rest-adapter (which json-api adapter extends) we import two utils from ember-fetch

import serializeQueryParams from 'ember-fetch/utils/serialize-query-params';
import determineBodyPromise from 'ember-fetch/utils/determine-body-promise';

Effectively this means that users must include ember-fetch in their dependencies even if they use jQuery. I believe this just means we need a minor tweak to the dependencies check we already do.

The other option is to port those two utils into ember-data to avoid needing to force folks to install ember-fetch if they don't want/need it.

@rwjblue do you have thoughts?

@dcyriller
Copy link
Contributor

Indeed, two ember-fetch's modules are imported in rest (and then json-api) adapter: NullVoxPopuli#3

@NullVoxPopuli
Copy link
Sponsor Contributor Author

shakes fist at lock files

NullVoxPopuli and others added 3 commits March 9, 2019 15:23
move ember-fetch to a peer-dependency

fix jsonapi adapter tests

add tests for fetch
* Fix lint error

* Fix `_requireBuildPackages` error

* Reset yarn.lock from master

There seems to be some issues in the yarn.lock

To solve them I ran:
- `git checkout master yarn.lock`
- `yarn`

Throw an error in case of dependency mismatch (#2)

* Throw an error in case of dependency mismatch

An app (or addon) that consumes ember-data should have:
- either jquery
- or ember-fetch version 6.0.0 or above

Throw a build error if it's not the case.

* Remove ember-fetch as a peerDependency

Move ember-fetch to dependencies (#3)

Indeed, two `ember-fetch`'s modules are imported in rest-adapter.
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.

4 participants