Replace jsdom with browser-env in browser recipe
#1054
Conversation
docs/recipes/browser-testing.md
Outdated
| ``` | ||
| $ npm install --save-dev jsdom | ||
| $ npm install --save-dev node-browser-environment |
There was a problem hiding this comment.
That's a very verbose module name. Why not simply browser-env? Node is already implied.
There was a problem hiding this comment.
I chose it because I thought that was the exact string most people would search for trying to find something like this.
browser-env is already taken, in hind site browser-environment would probably of been better. I'm reluctant to change it now as it's already had a fair amount of downloads and I don't think this can be changed in a backwards compatible way.
There was a problem hiding this comment.
It's pretty easy. You just bump major on the existing one and deprecate with a message saying it's been renamed. We bump major first so the deprecation notice doesn't annoy existing users until they bump a version. I would personally do it as that module is going to be used a lot and browser-environment would look nicer in the import statement, but your choice ;)
There was a problem hiding this comment.
Actually, https://github.com/CamShaft/browser-env is deprecated, so you could just request the module name.
There was a problem hiding this comment.
Good spot on the deprecation, yeah I'll rename it. If you wanna hold off on merging this for now I'll update you when it's sorted 👍
Quick Q: Why bump major first? Wouldn't it be better to notify existing users? If they've installed with the default ^ range they'll be expecting to get SemVer minor/patch updates automatically.
There was a problem hiding this comment.
@lukechilds Not very relevant in this situation, as it's a top-level module, so maybe not necessary, but when your module is deep in a dependency tree, people will start opening issues on every module that depends on it somewhere in the dependency tree about updating, which is very annoying for the maintainers and a lot of wasted time. Users think warnings are critical.
There was a problem hiding this comment.
@sindresorhus node-browser-environment is dead, long live browser-env! 🎉
docs/recipes/browser-testing.md
Outdated
| global.document = require('jsdom').jsdom('<body></body>'); | ||
| global.window = document.defaultView; | ||
| global.navigator = window.navigator; | ||
| require('node-browser-environment')(); |
There was a problem hiding this comment.
Should use import, not require.
docs/recipes/browser-testing.md
Outdated
| By default `node-browser-environment` will add all global browser variables to node's global scope creating a full browser environment. This should have good compatibility with most front end libraries, however it's generally not a good idea to create loads of global variables if you don't need to. If you know exactly which browser variables you need you can pass an array of required properties. | ||
|
|
||
| ```js | ||
| require('node-browser-environment')(['window', 'document', 'navigator']); |
docs/recipes/browser-testing.md
Outdated
| ``` | ||
|
|
||
| ## Configure tests to use jsdom | ||
| By default `node-browser-environment` will add all global browser variables to node's global scope creating a full browser environment. This should have good compatibility with most front end libraries, however it's generally not a good idea to create loads of global variables if you don't need to. If you know exactly which browser variables you need you can pass an array of required properties. |
There was a problem hiding this comment.
node => Node.js
front end => front-end
loads of => lots of
|
@lukechilds Are you committed to keeping this module up to date with |
|
@sindresorhus Yep, I've pinned the version number for I've got no intention of abandoning this module but I'm happy to add you or anyone else on the AVA team as a maintainer if you want. Working on your requested changes now 👍 |
|
@sindresorhus Done requested changes 🙂 |
|
This should be good to go now. Might be worth also adding a simpler example just to illustrate the browser globals are available? The current React example hides all the DOM stuff inside React. |
|
I'm thinking something along the lines of:
import test from 'ava';
test('Insert to DOM', t => {
const div = document.createElement('div');
document.body.appendChild(div);
t.is(document.querySelector('div'), div);
});It could be as well as the current React test or instead of. What do you think? |
|
Good idea. Should be in addition to the React one. |
|
@sindresorhus Done 👍 |
sindresorhus
changed the title
jsdom with browser-env in browser recipe
|
Thanks :) |
Fixes #882