-
Notifications
You must be signed in to change notification settings - Fork 3.2k
[CP Stg] Fix CORS errors in desktop application #7665
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
Changes from 6 commits
2561aea
c77bbef
759af3a
d3e7dd5
3cf3eb9
e47cc83
5d7c549
0424b45
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
// This variable is injected into package.json by electron-builder via the extraMetadata field (specified in electron.config.js) | ||
// It will be `PROD` on production, `STG` on staging, and `undefined` on dev (because dev doesn't use electron-builder) | ||
const {electronEnvironment} = require('../package.json'); | ||
const ENVIRONMENT = require('../src/CONST/ENVIRONMENT'); | ||
|
||
/** | ||
* @returns {String} – One of ['PROD', 'STG', 'DEV'] | ||
*/ | ||
function getEnvironment() { | ||
// If we are on dev, then the NODE_ENV environment variable will be present (set by the executing shell in start.js) | ||
if (process.env.NODE_ENV === 'development') { | ||
return ENVIRONMENT.DEV; | ||
} | ||
|
||
// Otherwise, use the environment injected into package.json by electron-builder | ||
return electronEnvironment; | ||
} | ||
|
||
function isDev() { | ||
return getEnvironment() === ENVIRONMENT.DEV; | ||
} | ||
|
||
function isProd() { | ||
return getEnvironment() === ENVIRONMENT.PRODUCTION; | ||
} | ||
|
||
module.exports = { | ||
getEnvironment, | ||
roryabraham marked this conversation as resolved.
Show resolved
Hide resolved
|
||
isDev, | ||
isProd, | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,10 +11,10 @@ const serve = require('electron-serve'); | |
const contextMenu = require('electron-context-menu'); | ||
const {autoUpdater} = require('electron-updater'); | ||
const log = require('electron-log'); | ||
const ELECTRON_ENVIRONMENT = require('./ELECTRON_ENVIRONMENT'); | ||
const ELECTRON_EVENTS = require('./ELECTRON_EVENTS'); | ||
const checkForUpdates = require('../src/libs/checkForUpdates'); | ||
|
||
const isDev = process.env.NODE_ENV === 'development'; | ||
const port = process.env.PORT || 8080; | ||
|
||
/** | ||
|
@@ -41,7 +41,7 @@ autoUpdater.logger.transports.file.level = 'info'; | |
_.assign(console, log.functions); | ||
|
||
// setup Hot reload | ||
if (isDev) { | ||
if (ELECTRON_ENVIRONMENT.isDev()) { | ||
try { | ||
require('electron-reloader')(module, { | ||
watchRenderer: false, | ||
|
@@ -124,12 +124,12 @@ const electronUpdater = browserWindow => ({ | |
}); | ||
|
||
const mainWindow = (() => { | ||
const loadURL = isDev | ||
const loadURL = ELECTRON_ENVIRONMENT.isDev() | ||
? win => win.loadURL(`http://localhost:${port}`) | ||
: serve({directory: `${__dirname}/../dist`}); | ||
|
||
// Prod and staging set the icon in the electron-builder config, so only update it here for dev | ||
if (isDev) { | ||
if (ELECTRON_ENVIRONMENT.isDev()) { | ||
app.dock.setIcon(`${__dirname}/icon-dev.png`); | ||
app.setName('New Expensify'); | ||
} | ||
|
@@ -147,8 +147,29 @@ const mainWindow = (() => { | |
titleBarStyle: 'hidden', | ||
}); | ||
|
||
if (!ELECTRON_ENVIRONMENT.isDev()) { | ||
const newDotURL = ELECTRON_ENVIRONMENT.isProd() ? 'https://new.expensify.com' : 'https://staging.new.expensify.com'; | ||
|
||
// Modify the request origin for requests sent to our API | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you please add more to this comment to explain "why?". Currently, it is only explaining "what". There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added a comment above this block. Give it a read and let me know if you think it's clear enough, or if it leaves you with more questions. |
||
const validDestinationFilters = {urls: ['https://*.expensify.com/*']}; | ||
browserWindow.webContents.session.webRequest.onBeforeSendHeaders(validDestinationFilters, (details, callback) => { | ||
// eslint-disable-next-line no-param-reassign | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you could get around this by using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I actually took one out of your playbook from this PR ... this code will be executed before and after every api request, so performance matters. Using the spread operator would cause us to perform an unnecessary shallow copy of all the headers for every request twice per request, which seems less good than just overwriting a couple properties. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, that's probably a good call. I had also thought of that, and the fact that this is done on every request is a good reason to keep it very performant. The only caveat I will say is that in my PR, I think the spread operator is not performant because of how Babel chooses to transpile it into Something else to consider. In my PR, since there was a reducer used along with the spread operator, it could have been compounding the problem. This is mentioned specifically in this SO: https://stackoverflow.com/questions/55843097/does-spread-operator-affect-performance
So, I'm not really sure that using a spread operator here is such a bad thing. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks, I wasn't aware of that context, especially regarding Babel. Very good to know about watching out for a spread operator in reducers! (maybe we could create a lint rule for that?)
That makes sense – I agree it might not create a performance problem to use the spread operator here. But still I would lean towards leaving this as-is because I think it will be "technically" more performant in a very performance-critical place, which seems worth the couple |
||
details.requestHeaders.origin = newDotURL; | ||
// eslint-disable-next-line no-param-reassign | ||
details.requestHeaders.referer = newDotURL; | ||
callback({requestHeaders: details.requestHeaders}); | ||
}); | ||
|
||
// Modify access-control-allow-origin header for the response | ||
browserWindow.webContents.session.webRequest.onHeadersReceived(validDestinationFilters, (details, callback) => { | ||
// eslint-disable-next-line no-param-reassign | ||
details.responseHeaders['access-control-allow-origin'] = ['app://-']; | ||
callback({responseHeaders: details.responseHeaders}); | ||
}); | ||
} | ||
|
||
// Prod and staging overwrite the app name in the electron-builder config, so only update it here for dev | ||
if (isDev) { | ||
if (ELECTRON_ENVIRONMENT.isDev()) { | ||
browserWindow.setTitle('New Expensify'); | ||
} | ||
|
||
|
@@ -286,7 +307,7 @@ const mainWindow = (() => { | |
|
||
// Start checking for JS updates | ||
.then((browserWindow) => { | ||
if (isDev) { | ||
if (ELECTRON_ENVIRONMENT.isDev()) { | ||
return; | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
module.exports = { | ||
DEV: 'DEV', | ||
STAGING: 'STG', | ||
PRODUCTION: 'PROD', | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,7 @@ | ||
import lodashGet from 'lodash/get'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why did the name of this file change? I don't think it should be changed. The only time we should ever have an There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This got renamed because of CJS and ESM modules not playing nice together. I wanted to use
By moving import CONST from '../CONST'; And JS will automatically grab If you want me to revert this, I could just create a separate constant directly in the |
||
import Config from 'react-native-config'; | ||
import * as Url from './libs/Url'; | ||
import ENVIRONMENT from './ENVIRONMENT'; | ||
import * as Url from '../libs/Url'; | ||
|
||
const CLOUDFRONT_URL = 'https://d2k5nsl2zxldvw.cloudfront.net'; | ||
const ACTIVE_ENVIRONMENT_NEW_EXPENSIFY_URL = Url.addTrailingForwardSlash(lodashGet(Config, 'EXPENSIFY_URL_CASH', 'https://new.expensify.com')); | ||
|
@@ -390,12 +391,6 @@ const CONST = { | |
ADMIN: '[email protected]', | ||
}, | ||
|
||
ENVIRONMENT: { | ||
DEV: 'DEV', | ||
STAGING: 'STG', | ||
PRODUCTION: 'PROD', | ||
}, | ||
|
||
// Used to delay the initial fetching of reportActions when the app first inits or reconnects (e.g. returning | ||
// from backgound). The times are based on how long it generally seems to take for the app to become interactive | ||
// in each scenario. | ||
|
@@ -614,4 +609,6 @@ const CONST = { | |
}, | ||
}; | ||
|
||
CONST.ENVIRONMENT = ENVIRONMENT; | ||
|
||
export default CONST; |
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.
This is where it still remains a bit weird for me. The
electronEnvironment
variable will only contain PROD or STAG. The development environment comes fromprocess.env.NODE_ENV
. Can't they all come from the same place? Like, can this code here be something like...However, even in that code... if
process.env.NODE_ENV
is notdevelopment
, then what is it? Maybe this logic can be cleaned up a little more.The goal here would be that
getEnvironment()
(below) would only be looking at a single place to determine the environment.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 sounds like a good idea. As I understand it
process.env.NODE_ENV
would be undefined outside of development because: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 would love to achieve this goal, but I don't see how. You see,
electron.config.js
would actually be better-namedelectron-builder.config.js
, because it's not actually used by Electron itself – onlyelectron-builder
. So there would never be a time whereprocess.env.NODE_ENV === 'development'
inelectron.config.js
.When we run the development app here, we can easily set an environment variable such as
NODE_ENV
in the runtime, and that will be available to the Electron main process. However, when we bundle an app usingelectron-builder
(or really, create any executable file), we can't "bake" an environment variable in AFAIK. If I were to do something like this:Then when I install and run the bundled Electron app, and try to access
process.env.NODE_ENV
, it will be undefined. You have to set the environment variable from the shell or environment that runs the executable.However,
electron-builder
does provide a weird workaround. You can set data in theextraMetadata
field in the configuration file, and thenelectron-builder
will inject those variables into yourpackage.json
, which is available to the application at runtime.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 makes sense to me.