don't load css in node environment #2492
Conversation
src/utils/device.js
Outdated
| /** | ||
| * Check if running in a browser. | ||
| */ | ||
| function isBrowser () { |
There was a problem hiding this comment.
Does this need to be a function? I'd do maybe:
module.exports.isBrowserEnvironment = !process;
module.exports.isNodeEnvironment = !!process;
There was a problem hiding this comment.
We can add a unit test for these in Karma.
There was a problem hiding this comment.
I was matching the convention in the file. isIOS () and isGearVR () are functions when I don't think they need to be either.
There was a problem hiding this comment.
Yeah, I guess they don't need to be. Maybe later we can make those properties.
There was a problem hiding this comment.
To clear up, I mean I think we should make isBrowserEnvironment + isNodeEnvironment properties now, and make isIOS + isGearVR properties later too.
src/utils/device.js
Outdated
| /** | ||
| * Check if running in node on the server. | ||
| */ | ||
| module.exports.isNode = function () { |
There was a problem hiding this comment.
We can also add a unit test for these in our Node tests
|
I guess the advantage of having them as function is that the logic to determine if the environment isGearVR, isOS, isNode does not run unless is required. |
|
These are always run though since we call them from index.js and a-scene.js. The disadvantage is that we are calling them multiple times. |
kellyselden
force-pushed
the
node-css
branch
2 times, most recently
from
36c2589
to
c124243
Compare
|
I changed to value exports instead of functions. When adding tests. I found out that not only is |
src/utils/device.js
Outdated
| /** | ||
| * Check if running in a browser. | ||
| */ | ||
| // we need to check a node api that isn't mocked on either side |
There was a problem hiding this comment.
I'm not sure if I did the commenting right. I was trying to separate the public comments that go in the api docs, from private implementation comments.
There was a problem hiding this comment.
I think you can just use normal comments throughout for these all properties. We don't actually generate docs from this, it's mostly all developer comments, but in JSDoc form. (with preference to capitalized / ending in period comments)
src/utils/device.js
Outdated
| /** | ||
| * Check if running in a browser or spoofed browser. | ||
| */ | ||
| var isBrowserLikeEnvironment = isBrowserEnvironment || isBundlerEnvironment; |
There was a problem hiding this comment.
Can we narrow it to two?
isBrowserEnvironment- true if the environment is even browser-like.isNodeEnvironment- Not browser-like.
And then we can export straight up in two lines:
module.exports.isBrowserEnvironment = !process || !!(process && process.browser);
module.exports.isNodeEnvironment = !!module.exports.isBrowserEnvironment
tests/node/test.js
Outdated
| const Module = require('module'); | ||
| const sinon = require('sinon'); | ||
|
|
||
| const _require = Module.prototype.require; | ||
|
|
||
| describe('node acceptance tests', function () { |
There was a problem hiding this comment.
can we run mocha in tdd mode to match the other tests?
There was a problem hiding this comment.
Added in #2496 and will rebase.
|
|
||
| suite('environment', function () { | ||
| test('isNodeEnvironment is false for browser tests', function () { | ||
| assert.strictEqual(device.isNodeEnvironment, false); |
There was a problem hiding this comment.
could use assert.ok or assert.notOk
There was a problem hiding this comment.
I would like to test the correct type as well. I don't want to accidently return a truthy/falsy object (which I almost did: process && process.browser was returning undefined instead of false).
|
Can rebase this now |
|
rebased and updated. |
|
Thanks! Love the test code deletions. |
Now that #2484 has landed, we can begin to iterate on the node experience and make it better. This removes the need to mock
requireto ignore css requiring in node (which is a bundler-only feature).