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.

Sign up for GitHub

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

Jump to bottom

Specify GET Method to Load templates #437

Merged
merged 3 commits into from
Oct 24, 2017
Merged

Specify GET Method to Load templates #437

merged 3 commits into from
Oct 24, 2017

Conversation

santiph
Copy link
Contributor

@santiph santiph commented Oct 19, 2017

RemoteStorage is using POST to load templates
Issue: #436

@santiph santiph changed the title Specify GET Method to Load templates WIP: Specify GET Method to Load templates Oct 19, 2017
santiph
santiph commented Oct 19, 2017
View reviewed changes
src/storage_manager/model/RemoteStorage.js Outdated
@@ -74,7 +74,7 @@ module.exports = require('backbone').Model.extend({
},

load(keys, clb) {
this.request(this.get('urlLoad'), {body: {keys}}, clb);
this.request(this.get('urlLoad'), {body: {keys}, method: 'get'}, clb);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Body on GET?? @.@

@artf
Copy link
Member

artf commented Oct 20, 2017

One of the ideas of urlLoad is too get only requested resources by using keys (eg. ["html", "css", ...]). Unfortunately, the fetch specs don't allow an 'easy' way to add query parameters (JakeChampion/fetch#256) so this is why I left it as it is 😬
So before merging, I'd say we need to transform that array in a query string (eg. keys[]=val&key[]=val2)

@santiph santiph force-pushed the patch/remoteStorage-Load-method branch from 7a800e8 to 68b0596 Compare October 21, 2017 05:15
@santiph
Copy link
Contributor Author

santiph commented Oct 21, 2017

What about a comma separated list of keys? /templates/42?keys=assets,css,styles,html,components

@santiph santiph changed the title WIP: Specify GET Method to Load templates Specify GET Method to Load templates Oct 21, 2017
@artf
Copy link
Member

artf commented Oct 23, 2017

What do you think about removing keys completely? If anybody need to fetch one resource or more all you have to do is to update urlLoad with your keys and logic.

I don't even see the point of adding bodilessMethods check, grapesjs's fetch polyfill was made to fit its own needs, if someone will use it for other scenarios it's not my responsability to check their requests, moreover you're applying the same logic twice (in RemoteStorage.js and fetch.js) :)

So, if you just remove the keys, from the load method, I'm gonna merge it

@artf artf merged commit 88c262b into GrapesJS:dev Oct 24, 2017
@artf
Copy link
Member

artf commented Oct 24, 2017

Thanks @santiph 👍

@santiph
Copy link
Contributor Author

santiph commented Oct 24, 2017

Thanks to you, @artf ;)
Would you mind to publish a new version of grapesjs into npm?

@santiph santiph mentioned this pull request Oct 24, 2017
Closed
@artf
Copy link
Member

artf commented Oct 25, 2017

published

@lock
Copy link

lock bot commented Sep 17, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot added the outdated label Sep 17, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Sep 17, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@artf artf artf approved these changes

Assignees
No one assigned
Labels
outdated
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@santiph @artf