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

JSON requests for local apps #41

Closed
wants to merge 2 commits into from
Closed

JSON requests for local apps #41

wants to merge 2 commits into from

Conversation

Ali-Khatami
Copy link
Contributor

I added a few properties (with proper comment documentation) and modified the registerApps method to allow a container developer to specify whether or not they would like to load "local" apps (apps hosted on the same domain as the container) through JSON vs JSONp through the use of allowLocalDomainJSON=true. There is also the ability to specify what the domain check will use via localDomain.

Modifying localDomain allows a developer to do something as follows:

Container URL: https://sub.greatdomain.co.jp
APP URL: https://f2app.greatdomain.co.jp

F2.init({ allowLocalDomainJSON: true, localDomain: "greatdomain.co.jp" });

Container URL: https://sub.greatdomain.com
APP URL: https://f2app.greatdomain.com

F2.init({ allowLocalDomainJSON: true, localDomain: "greatdomain.com" });
....

  • Modified/Added logic in F2.registerApps to read containerConfig properties allowLocalDomainJSON and localDomain to determine whether to request an app via JSON or JSONp

…fig.

* Modified/Added logic in F2.registerApps to read containerConfig properties allowLocalDomain and localdomain to determine whether to request an app via JSON or JSONp
@ilinkuo
Copy link

ilinkuo commented Mar 2, 2013

Uh, is this really necessary to expose as an option? Why not make the loader smarter -- try a local app as JSONP and if that fails, try as JSON, and if that works, set a field in the appConfig to use JSON.

But I don't really see what this "feature" gains except possibly faster prototyping. All f2 apps really need to be able to respond to JSONP in order to work on other domains.. Making them also respond to JSON would usually be an additional burden. Now, if you only want apps to work on your own domain, then that's a legitimate use of this feature, but in that case, it's a property of the app rather than the container, and should be in the app manifest rather than in the app config.

@markhealey
Copy link
Member

This definitely has implications for the F2 spec because this means the AppManifest takes on 2 forms—with and without the JSONP function wrapper.

Separately, jQuery used to have logic in the $.ajax function to automatically switch to json from jsonp if the request URL was on the same base domain—I think it's still there. Long/short is, if we add this support, I think the internal loader should make the on-the-fly switch to json using POST.

More details here:
http://forum.jquery.com/topic/make-json-vs-jsonp-request-depending-on-url

@Ali-Khatami
Copy link
Contributor Author

If all developers want to support multiple formats great, the intention of the allowLocalDomainJSON parameter was to not force developers to support JSON and JSONp. Also it should be noted that the container developer is the one in control of these parameters and also controls the local apps, which means it should be their decision whether or not they want to use JSON, not F2s decision.

At the very least you would need to provide the localDomain parameter if you want F2 to be smart enough to check the domain and determine or you will run into issues with apps on a different sub domain than the container.

I should also add that developers who host apps or containers that request apps, or are requested, across continents would probably not be in favor of F2 attempting the transmission of json then going to jsonp after (every second/millisecond counts).

Those are my justifications for adding the functionality that I did. I am certainly open to more discussion on this.

@markhealey - I haven't seen any evidence of jQuery making the switch automatically from json to jsonp. I'm sure we could through together some test cases though.

@brianbaker
Copy link
Member

@markhealey, @Ali-Khatami - jQuery will perform an XHR request when the protocol/host/port matches instead of doing a jsonp request. This can be seen in Chrome if you download the basic app template (http://docs.openf2.org/app-development.html#get-started). Here's the source code for it: https://github.com/jquery/jquery/blob/master/src/ajax.js#L442 The 'jsonp' transport delgates off to the 'script' transport, and the 'script' transport only applies to cross domain requests (https://github.com/jquery/jquery/blob/master/src/ajax/jsonp.js#L78 https://github.com/jquery/jquery/blob/master/src/ajax/script.js#L30)

@markhealey
Copy link
Member

Nice sleuthing. So, @brianbaker, is the only thing we're missing here the lack of POST support?

markhealey pushed a commit that referenced this pull request Mar 11, 2013
@markhealey
Copy link
Member

@Ali-Khatami, would this commit 28053bc work for your needs?

https://github.com/OpenF2/F2/blob/issue41/sdk/src/container.js#L550

Basically, I added a check on the manifestUrl defined in the AppConfig to see if it's "local" or not. If the manifest URL is local, the $.ajax.type is set to POST during F2.registerApps().

(Since I added 3 new private methods, I also updated the SDK docs in the issue41 branch.)

@markhealey
Copy link
Member

Also, my test case was:

@markhealey markhealey mentioned this pull request Mar 13, 2013
@Ali-Khatami
Copy link
Contributor Author

Closing this based on Mark's test cases.

@markhealey
Copy link
Member

Wait, so you undid my changes in my proposed commit? Or not…?

@Ali-Khatami
Copy link
Contributor Author

I probably shouldn't have closed this issue seeing as how you have changes to change local requests to POST. I just deleted this pull request because I deleted the repo a while ago. Sorry about the confusion. Let me know how you want to proceed.

@markhealey
Copy link
Member

Opened #59 to track this enhancement.

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.

None yet

4 participants