-
Notifications
You must be signed in to change notification settings - Fork 30
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
Have made cross origin login fix #302
Conversation
This removes code that is never run with is_dev_server true. This fixes the addition of haveMadeCrossOriginLogin_ flag to the URL query elsewhere, which also appears to be redundant.
It seems this was only used previously in some dev settings when running the OpenLayers Viewer independently of the iviewer as a whole. When the iviewer is deployed via the webpack dev server, redirection (if not logged in) is handled in index-dev.html
Looks good to me. |
let prefix = | ||
typeof req[URI_PREFIX] === 'string' ? | ||
Misc.prepareURI(req[URI_PREFIX]) : ""; | ||
__webpack_public_path__ = prefix + '/static/' + PLUGIN_NAME + '/'; |
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.
In general, it seems like you've stripped things out regarding for when dev_server
is true which seems safe enough, but isn't this clause for the non-dev_server
case?
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.
I don't fully understand all of the use cases here, but as far as I can see, this code was hard-coded NOT to run (since is_dev_server = true;
came right before that if).
So I don't know when that code has ever run.
It's only job is to set __webpack_public_path__
.Webpack dev server isn't used in production. Webpack is used to package the JS bundle, but we've been doing that a lot without this code, so I can only assume this isn't needed in our current workflows.
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.
Urgh. Thanks for the cleanup 👍
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.
see #142 for the introduction of this.
Merging and leaving someone else to suggest release. |
Fixes #283
To test:
haveMadeCrossOriginLogin_
flag