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

[FEAT fetch] use ember-fetch instead of jQuery #5386

Closed
wants to merge 9 commits into from

Conversation

tchak
Copy link
Member

@tchak tchak commented Mar 17, 2018

depends on #5375, #5385 and #5375
will fix #5320

import Ember from 'ember';
import fetch, { Response } from 'fetch';
//FIXME: We need to add a direct export of `serializeQueryParams`
import { serializeQueryParams } from 'ember-fetch/mixins/adapter-fetch';
Copy link
Contributor

Choose a reason for hiding this comment

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

I've been wondering where the best place for this to live is. In a jQuery-less world, $.param() is gone and Ember should have some replacement.

Copy link
Member Author

Choose a reason for hiding this comment

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

I feel like ember-fetch is good place for now. I am planing on doing a PR to move it out of ember-fetch/mixins/adapter-fetch. But a broader discussion is needed about fetch usage: fetch on it own is not a suitable replacement for $.ajax in my opinion. fetch is too low level.

Copy link
Member Author

Choose a reason for hiding this comment

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

Another option might be to use URLSearchParams provided by ember-url (with polyfills). It is a standard, but might be less convenient then a $.param().

Copy link
Contributor

Choose a reason for hiding this comment

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

what's better? URLSearchParams from ember-url, or serializerQueryParams from the fetch adapter?

Copy link
Member Author

Choose a reason for hiding this comment

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

Using URLSearchParams is not an option after investigation – it won't serialise nested params. I think wee should expose something like import { param } from 'ember-fetch';

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@tchak ember-fetch has been updated!

@btecu
Copy link
Contributor

btecu commented Jun 10, 2018

@tchak What's the status? Seems to be the last step for being able to use ED without jQuery.

@tchak tchak mentioned this pull request Sep 8, 2018
@runspired runspired changed the title [WIP] Use fetch [FEAT fetch] use ember-fetch instead of jQuery Sep 8, 2018
package.json Outdated
@@ -48,6 +48,7 @@
"ember-cli-string-utils": "^1.1.0",
"ember-cli-test-info": "^1.0.0",
"ember-cli-version-checker": "^2.1.2",
"ember-fetch": "^5.1.3",
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should we make this a peerDependency instead to avoid trolling folks with multiple versions, and a devDependency for our own use.

Copy link
Contributor

Choose a reason for hiding this comment

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

this still needs to be removed from here. this was probably my bad when I did partial work on this.

@runspired
Copy link
Contributor

@tchak @NullVoxPopuli think this needs updated for code style, there's also some broken tests

@NullVoxPopuli
Copy link
Contributor

NullVoxPopuli commented Sep 8, 2018

I'll take a look. :)
unless @tchak is already working on it :)

@tchak
Copy link
Member Author

tchak commented Sep 9, 2018

I probably won't be available today match, but I should be able to help if needed tomorrow

@NullVoxPopuli
Copy link
Contributor

copying this from the other PR:

I think there should probably be a good amount of tests for fetch, which don't exist yet.
also, with ember fetch, there is this function: mungOptionsForFetch

So, I think for this feature to be complete, we need to find all the request url / headers / whatever related tests and make fetch versions of them.

@tchak
Copy link
Member Author

tchak commented Sep 10, 2018

There is a couple of external tests failing. Ember Observer seems like a timeout. Ilios Frontend looks like some genuine errors. I will have to look in to it more closely.

@runspired
Copy link
Contributor

@tchak may also be flakiness, it passed when I restarted the job. cc @jrjohnson

@jrjohnson
Copy link
Contributor

I can't see the failure, but we've been struggling terribly with flaky tests the last couple of days for reasons we haven't tracked down yet. If the error was something like https://github.com/ilios/frontend/issues/4046 then this may be the issue.

@NullVoxPopuli
Copy link
Contributor

build passed! (but hasn't reported back to github for some reason)

@runspired
Copy link
Contributor

@NullVoxPopuli anything remaining? I noticed this:

I think there should probably be a good amount of tests for fetch, which don't exist yet.
also, with ember fetch, there is this function: mungOptionsForFetch

So, I think for this feature to be complete, we need to find all the request url / headers / whatever related tests and make fetch versions of them.

@NullVoxPopuli
Copy link
Contributor

@runspired good catch --

this file: https://github.com/emberjs/data/pull/5386/files#diff-038a1249383849dd6526ee97d193e925R90

the ajax file needs a fetch equivalent.

also, are coverage artifacts outputted somewhere? my hunch is that a lot of the new fetch stuff is missing most coverage

@NullVoxPopuli
Copy link
Contributor

tests and ember-fetch dep removed: tchak#3

@mansona
Copy link
Member

mansona commented Dec 3, 2018

@tchak I know it might seem a bit hacky but what if you did something in the index.js that used resolve to check what version ember-fetch is actually installed and throw an error if it's too low?

@NullVoxPopuli
Copy link
Contributor

do peerDependencies not solve this?

@mansona
Copy link
Member

mansona commented Dec 3, 2018

@NullVoxPopuli I think it's just a warning when it comes to peerDependencies 🤔 but I could be wrong

@xg-wang
Copy link

xg-wang commented Dec 4, 2018

peerDependency just writes a warning. Forcing it to an error can be done by ember-cli-version-checker

@runspired
Copy link
Contributor

@xg-wang is correct, and likely that’s what we should do.

@runspired
Copy link
Contributor

I’m a bit hesitant to do something that forces folks to install ember-fetch as much as I want to eliminate jQuery. We should likely make whether it is required “optional” and only use version checker to enforce if the flag is not set to drop it.

@tchak
Copy link
Member Author

tchak commented Dec 4, 2018

Si it should throw an error only if no jQuery support is enabled and ember-fetch is absent or lower than required version, correct?

@runspired
Copy link
Contributor

@tchak I was thinking more broad: as in it should likely be the default that without jquery we expect fetch, but folks should be able to opt out of both.

@xg-wang
Copy link

xg-wang commented Dec 5, 2018

shouldIncludeChildAddon(childAddon) {
      if (childAddon.name === 'ember-fetch') {
        return !HAS_JQUERY;
      } else {
        return this._super.shouldIncludeChildAddon.apply(this, arguments);
      }
    }

I think this could do it. To check top level & sibling ember-fetch existence should be done in ember-fetch self's cli hook.

@dcyriller
Copy link
Contributor

I’m a bit hesitant to do something that forces folks to install ember-fetch as much as I want to eliminate jQuery
folks should be able to opt out of both

@runspired how do you see the opt out strategy? I guess the fallback would be on a non-polyfilled version of fetch (what about fastboot?). In config/environment.js, we could toggle the native fetch strategy this way?

module.exports = function(environment) {
  var ENV = {
    'ember-data': {
      useNativeFetch: true // defaults to false
    }
  }
}

@tchak I've done a PR (targeting yours) to throw an error in case of dependency mismatch.

@simonihmig
Copy link
Contributor

The quest to remove jQuery by default as per RFC386 is making good progress. Part of it is also resolving this issue.

So is there a chance we can land this in the near future? Ideally in a forthcoming 3.9 release? (which is the version of Ember which will add the additional jQuery deprecations)

@NullVoxPopuli
Copy link
Contributor

where can we view the status of this?

@runspired
Copy link
Contributor

@runspired how do you see the opt out strategy? I guess the fallback would be on a non-polyfilled version of fetch (what about fastboot?). In config/environment.js, we could toggle the native fetch strategy this way?

@dcyriller I think i'm going to skip on this requirement for now and add it as a feature later if folks need. Is all that is needed to land a rebase at this point?

@runspired
Copy link
Contributor

Closing in favor of #5900

@runspired runspired closed this Mar 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[QUEST] Making jQuery Optional
10 participants