-
Notifications
You must be signed in to change notification settings - Fork 8
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
updates forceng to recent version of force.js from salesforce #10
base: master
Are you sure you want to change the base?
updates forceng to recent version of force.js from salesforce #10
Conversation
… mobile sdk version 7++
@@ -37,7 +37,7 @@ angular.module('forceng', []) | |||
|
|||
// Only required when using REST APIs in an app hosted on your own server to avoid cross domain policy issues | |||
// To override default, pass proxyURL in init(props) | |||
proxyURL, | |||
proxyURL = baseURL, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are we defaulting the proxyURL to baseURL?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
finally see your comment. 💃
@frankvlocity @jegauten The changes here seem to force the file to only assume it will be used on mobile. Can we do it in a way where we don't force it to only assume it will be used on mobile because for example we'll use this in Omni in omniout? |
hi @mattgoldspink im not sure which part are you referring to but as far as i know most of the changes can support both mobile and web, though there is one change that allow api calls via plugin but only if the networkPlugin is present otherwise it will use the browser |
function getURLs() { | ||
return {proxyURL:proxyURL,oauthCallbackURL:oauthCallbackURL, useProxy: useProxy}; | ||
} | ||
|
||
function isOnMobile() { | ||
return true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jegauten RE: it being mobile only.
This line is what seemed to suggest it's going to be focussed on purely mobile. You're always returning true. Is this right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good catch. I don't think we use that. Let me check with team.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks Matt.
yes we dont use that we have a flag on our own in the app.js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we safely remove it so it's clear it's not needed/used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes it will be removed
Who has the permission to pull in the change and close this? @mattgoldspink @haxaco ? |
feature: forceng.js update
impact: salesforce api/request calls
scope: native mobile (iOS, Android)
changes:
- refactor logic on loginWithPlugin
- refactor error response from plugin to fix blank error message on failed request
- changed method in update method from post to patch
- other salesforce changes in force.js