-
Notifications
You must be signed in to change notification settings - Fork 224
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
fix(env): don't assume process
is defined
#692
Conversation
Most environments (webpack, browserify) by default define `process` as a shim for it. However, some environements don't supply it, so we should always guard against it not being defined fixes #691 (should be tested in a real angular app first)
shoot, this somehow makes browserify add an extra polyfill for |
@@ -10,7 +10,9 @@ var store = require('./store.js'); | |||
// proxies limit) | |||
var MAX_API_KEY_LENGTH = 500; | |||
var RESET_APP_DATA_TIMER = | |||
process.env.RESET_APP_DATA_TIMER && parseInt(process.env.RESET_APP_DATA_TIMER, 10) || | |||
(typeof process !== 'undefined' && |
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 one is actually only needed in integration tests and could be turned into a hidden option (__resetAppDataTimer)
Can we make it so that the fix is inside Angular InstantSearch?
I am trying to find "the good way" to fix this in the community, as using process.env is common I am not sure of the right way to rewrite it. |
That could be an option, I’ll try that out |
I've tried to hotfix this into Angular InstantSearch directly with: // HOTFIX: define process
if (typeof window !== "undefined") {
(window as any).process = { env: {} };
} else if (typeof global !== "undefined") {
(global as any).process = { env: {} };
} As entry point before anything else, it does not work :/ |
Since the workaround works in user land and this is stalled on the fact that it adds an additional |
@Haroenv can we maybe re-open this issue as it still breaks any Webpack web-only build for those people that don't want to ship extra polyfills or shims? This fix would not only improve some broken builds right now but is also necessary if you want to support Webpack 5, as it'll get rid of node polyfills (see https://twitter.com/wSokra/status/1076126619146899462). It seemed like a good idea for now but like a necessity for the future, IMO :) |
Summary
Most environments (webpack, browserify) by default define
process
as a shim for it.However, some environements don't supply it, so we should always guard against it not being defined
fixes #691
(should be tested in a real angular app first)
Result
guards around every use of
process