kupriyanenko / jbone

JavaScript Library for Events and DOM manipulation. Replaces jQuery for Backbone (2.5kb gzipped)
http://jbone.js.org
MIT License
279 stars 35 forks source link

Support for universal apps #60

Closed SleepWalker closed 7 years ago

SleepWalker commented 7 years ago

Hi,

jBone injects window from global scope. This breaks code execution on node, when you require module:

ReferenceError: window is not defined
    at Object.<anonymous> (./node_modules/jbone/dist/jbone.js:1062:3)

I suggest to use something like typeof window === 'undefined' ? global : window to guard from env differences.

The references to document will be fixed too

SleepWalker commented 7 years ago

@kupriyanenko, @redaxmedia I can prepare PR, if you still maintaining this repo and will accept such improvement

henryruhs commented 7 years ago

Hello SleepWalker,

What is your use case for jBone under nodejs? Is this just an issue you have while using a bundler like webpack or requirejs? In requirejs you need a window mockup module, in webpack this should work:

new webpack.ProvidePlugin({
    $: "jbone",
    jBone: "jbone",
    "window.jBone": "jbone"
})
kupriyanenko commented 7 years ago

Hi @SleepWalker, what is the use case do you have? I see the exception in nodejs, but usually you need to have jsdom for normal using, like:

require('jsdom-global')();
const jBone = require('jbone');

console.log(jBone);

If you really need it, will be cool if you will make PR. Or I will look on it today/tomorrow.

SleepWalker commented 7 years ago

@redaxmedia, @kupriyanenko I want to use jbone in React app with server side rendering (ssr). I do not need jbone on backend, because the code, that needs DOM manipulations, of coarse, executes in browser. But during server side rendering step nodejs requires some modules, that depend on jbone and you'll get a ReferenceError, despite jbone won't be used during ssr.

So I see two solutions:

The second one is a little bit tricky (or even not possible) but it will give more informative errors like 'document is not defined' instead of Cannot read property 'querySelectorAll' of undefined or doc.querySelectorAll is not a function.

henryruhs commented 7 years ago

There is no need for libaries like jQuery or jBone while using React or Angular. Everything can be solved using ReactDOM and DOM Refs ... please open a issue on Stackoverflow I can resolve it there.

Further reading:

https://facebook.github.io/react/docs/refs-and-the-dom.html https://facebook.github.io/react/docs/react-dom.html

SleepWalker commented 7 years ago

@redaxmedia you are wrong. There is no need in libs like jQuery, but there is need in lightweight wrappers around verbose DOM apis.

It's stupid pattern to use only vanilla js for DOM manipulations in framework. This pattern may be ok for todo mvc or kitten homepage, but not for huge apps with complex, dynamic interface and third party integrations. Using vanilla dom api will break at least DRY principle and will couple your code to specific implementation and global variables. Please think about this, before answering on stackoverflow in future.

And big thanks for this tiny lib!

henryruhs commented 7 years ago

Sorry, but you are doing something wrong when you think you need a DOM wrapper.

SleepWalker commented 7 years ago

What's the reason to write:

if (typeof document !== 'undefined') {
    const el = document.getElementById('foo');
}

when you can simply write $('#el')? Yes, you can get an element using ref, but only in case, when you are in context of react, but what if you are executing feature test for flash support?

Why should you write all the prefixed events for e.g. fullscreen events if you can simply write $(window).on('fullscreenchange')? What about rAF? (I know, that jbone does not handle this particular case, but we are talking about wrappers)

Why should you write:

if (typeof document !== 'undefined' && document.body) {
    const fn = () => alert('foo');

    document.body.addEventListener('click', (event) => {
        fn(event);
        document.body.removeEventListener('click', fn);
    });
}

if you can simply write: $('body').one(() => alert('foo'));?

If you are integrating with the third party products, they often require jQuery for DOM manipulation. In my case I can use jbone in place of jQuery and don't load the whole jQuery, which is full of features, that nobody needs (btw this lib was, actually written to solve similar issues with backbone ;) ).

That's why I think, that the wrapper is a good idea. It may be used as a layer, that abstracts some browser incompatibilities and reduces the repetition in your code.

kupriyanenko commented 7 years ago

@SleepWalker I think it's not bad idea to use abstraction over DOM, because even in react applications you have a cases when you should manipulate with DON. I can fix initialization for jBone, it's ok and not big deal.

But when you will try to use jBone/jQuery (doesn't meter), you will have runtime errors, for example for $(global).on('click', () => { /* ... */ }) in any case you will have an error.

What do you think about importing jsdom for your sever environment?

henryruhs commented 7 years ago

You are using the document object in your examples... well, I never tried server-side rendering in React because I switched to Angular but I guess you get in trouble using document on the nodejs side.

I see no problem using:

this.refs.foo; instead of document.getElementById('foo');

I see no problem using Flux or react-global-events:

const Root = () => (
    <div id="app-root" {...listenFor('mouseUp', 'mouseDown')}>
        ...
        <MySubComponent />
    </div>
)

You are working with the fullscreen API? Use the scrennlfull.js wrapper.You doing other fancy stuff? Search for a react wrapper on the NPM registry!

SleepWalker commented 7 years ago

But when you will try to use jBone/jQuery (doesn't meter), you will have runtime errors, for example for $(global).on('click', () => { / ... / }) in any case you will have an error.

Yep, I'll get error. But this is fine. Because, when my code touches DOM on node side, this is really an error. The problem was, that jbone touched DOM before it was actually called by my code.

I can fix initialization for jBone, it's ok and not big deal.

If you think, that this goes out of the scope of your lib, then you not obliged to do this. I've suggested this feature, because it may be useful. (btw. screenfull.js makes similar check in initialization code)


@redaxmedia thank you for suggestion. Probably, we have a little bit different codding habits. But this is another issue :) I don't know which is the best approach, but I love the mine one

kupriyanenko commented 7 years ago

@SleepWalker fix is released in version 1.2.1

kupriyanenko commented 7 years ago

@SleepWalker does it work for you?

SleepWalker commented 7 years ago

@kupriyanenko Yep. It works as expected. Thank you :+1: