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

Don't make duplicated requests through Ember Data #2087

Closed
wants to merge 2 commits into from

Conversation

kzys
Copy link
Contributor

@kzys kzys commented Jan 4, 2020

ember-data-storefront provides FastbootAdapter which caches XHRs in
Fastboot's Shoebox.

This will prevent duplicated requests on /crates and other
Ember Data-powered pages.

@rust-highfive
Copy link

r? @smarnach

(rust_highfive has picked a reviewer for you, use r? to override)

@kzys kzys force-pushed the ember-data-fastboot branch from a38f038 to a8c37cf Compare January 4, 2020 14:07
@jtgeibel jtgeibel mentioned this pull request Jan 4, 2020
19 tasks
@jtgeibel
Copy link
Member

jtgeibel commented Jan 5, 2020

Based on some concerns raised on Discord, lets reassign.

r? @locks

@rust-highfive rust-highfive assigned locks and unassigned smarnach Jan 5, 2020
@kzys
Copy link
Contributor Author

kzys commented Jan 6, 2020

From Discord:

@kzys I asked for second opinions on #2087 and I think we shouldn't bring in ember-data-storefront just for this

I don't have strong opinion on ember-data-storefront. If we don't want to add the package, can we inject fetch() into RESTAdapter instead? I'm not so sure how hard it would be.

https://github.com/emberjs/data/blob/94aef4dd61d8697636d23631441c19ff96e38f65/packages/adapter/addon/rest.js#L1057

@kzys
Copy link
Contributor Author

kzys commented Jan 12, 2020

Ping @locks?

@locks
Copy link
Contributor

locks commented Jan 14, 2020

Looking into it today!

@bors
Copy link
Contributor

bors commented Jan 14, 2020

☔ The latest upstream changes (presumably #2122) made this pull request unmergeable. Please resolve the merge conflicts.

@kzys
Copy link
Contributor Author

kzys commented Jan 21, 2020

@locks We could inject something that queries Fastboot's Shoebox on adapters by having our own initializer, but Ember's Adapter API doesn't provide a good hook point which we can utilize. Both ajax and _ajaxRequest are declared as private.

https://api.emberjs.com/ember-data/3.4/classes/DS.RESTAdapter?show=private

Surprisingly even ember-cli-fastboot is overriding _ajaxRequest.

https://github.com/ember-fastboot/ember-cli-fastboot/blob/6a5d6c6e38c30e5afda20db34de2ff5745b1416f/fastboot/initializers/ajax.js#L33

@kzys
Copy link
Contributor Author

kzys commented Feb 8, 2020

@locks - I'd like to hear your thoughts if you have some time.

@locks
Copy link
Contributor

locks commented Feb 14, 2020

Sorry, busy days lately. If I can't address this by Monday I will step out of the way.

@kzys kzys force-pushed the ember-data-fastboot branch 2 times, most recently from f4e1ce6 to a369d6e Compare February 19, 2020 06:03
@kzys
Copy link
Contributor Author

kzys commented Feb 19, 2020

I have removed ember-data-storefront on the last revision.

@kzys
Copy link
Contributor Author

kzys commented Feb 19, 2020

@jtgeibel @smarnach - Can you take a look since @locks is busy?

@kzys kzys force-pushed the ember-data-fastboot branch from c3be73c to a375466 Compare February 20, 2020 13:52
@jtgeibel
Copy link
Member

@kzys I had some trouble testing this in my staging area at https://jtgeibel-staging-crates-io.herokuapp.com/. (This PR is currently deployed there with USE_FASTBOOT=staging-experimental.) I was running into issues on both master and this branch.

So I focused on getting a better local environment set up. I've opened #2215 and I've got things working there now. I'd like to do a bit more testing, but this PR seems to work well for me locally (with and without fastboot) when merged with this PR.

I'm not sure what the issue with staging-experimental is when deployed to Heroku. In that environment I think maybe the requests from the fastboot server to the backend are being routed in a strange way. I'm not sure if they are going directly to the backend, locally through nginx, or externally through the Heroku router.

@kzys
Copy link
Contributor Author

kzys commented Feb 24, 2020

I'm not sure if they are going directly to the backend, locally through nginx, or externally through the Heroku router.

Hmm, I think it should be directly request the backend, but I'm not 100% sure.

@locks
Copy link
Contributor

locks commented Feb 26, 2020

I just found out about https://github.com/cardstack/ember-data-fastboot, which should help with the boilerplate.

@bors
Copy link
Contributor

bors commented Mar 21, 2020

☔ The latest upstream changes (presumably #2290) made this pull request unmergeable. Please resolve the merge conflicts.

@kzys
Copy link
Contributor Author

kzys commented Mar 22, 2020

@locks It is somewhat coupled with JSONAPISerializer, which we don't use.

Right now, this addon assumes that your application is using DS.JSONAPISerializer as your application's default serialization method.

@kzys
Copy link
Contributor Author

kzys commented Apr 5, 2020

I cannot keep up with all the refactorting the team is doing. Ping me once that is over...

@Turbo87 Turbo87 self-assigned this Apr 26, 2020
@Turbo87 Turbo87 force-pushed the ember-data-fastboot branch from a375466 to be21bb7 Compare April 26, 2020 20:20
kzys added 2 commits April 26, 2020 22:22
This will prevent duplicated requests on `/crates` and other
Ember Data-powered pages.
The options parameter is used to represent query parameters. So it must
be included to differentiate URLs with different query parameters.
@Turbo87 Turbo87 force-pushed the ember-data-fastboot branch from be21bb7 to 9578812 Compare April 26, 2020 20:22
@Turbo87 Turbo87 added C-internal 🔧 Category: Nonessential work that would make the codebase more consistent or clear and removed S-waiting-on-review labels Sep 10, 2021
@Turbo87
Copy link
Member

Turbo87 commented Jan 29, 2024

In #7301 we've removed most of the unused fastboot code, since it doesn't really seem well supported by Ember.js these days. Since this PR is building on top of fastboot, I'll close it for now. Thank you for your work anyway! :)

@Turbo87 Turbo87 closed this Jan 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-frontend 🐹 C-internal 🔧 Category: Nonessential work that would make the codebase more consistent or clear
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants