amp-bind: Remove web-worker fallback code#8956
amp-bind: Remove web-worker fallback code#8956dreamofabear merged 7 commits intoampproject:masterfrom
Conversation
1b77eff to
8b81c88
Compare
0ed6187 to
031990d
Compare
16c46cd to
7d1a3e0
Compare
| stub.returns('stubbed'); | ||
| evaluator.evaluateBindings({}); | ||
| expect(stub.calledOnce).to.be.true; | ||
| }); |
There was a problem hiding this comment.
This was moved from test-bind-impl.js.
|
/to @kmh287 |
kmh287
left a comment
There was a problem hiding this comment.
LGTM, sorry I missed this
| // Experiment check is bypassed on test mode -- make sure it isn't. | ||
| window.AMP_MODE = {test: false}; | ||
| env.sandbox.stub(getMode(), 'test', false); | ||
| expect(() => { |
There was a problem hiding this comment.
Is this the preferred way to stub the mode?
There was a problem hiding this comment.
Setting window.AMP_MODE is more common but stubbing avoids overwriting other mode properties (e.g. line 180).
IMO stubbing is generally better practice than setting global vars, but maybe there's something I'm missing. /cc @jridgewell
There was a problem hiding this comment.
Use AMP_MODE, we exported it specifically for this. You can use Object.assign if you're worried about conflicting properties.
dreamofabear
left a comment
There was a problem hiding this comment.
You didn't miss it, it was "WIP" for awhile. :)
| // Experiment check is bypassed on test mode -- make sure it isn't. | ||
| window.AMP_MODE = {test: false}; | ||
| env.sandbox.stub(getMode(), 'test', false); | ||
| expect(() => { |
There was a problem hiding this comment.
Setting window.AMP_MODE is more common but stubbing avoids overwriting other mode properties (e.g. line 180).
IMO stubbing is generally better practice than setting global vars, but maybe there's something I'm missing. /cc @jridgewell
fd2bc6d to
5502c86
Compare
5502c86 to
44ce829
Compare
Fixes #8955.