Can we make this safe for server-side use?

[JedWatson]

First up, thanks for this library :)

We've introduced it into Elemental UI but it has caused havoc with rendering React components on the server, because it references the window global when it is included, specifically this line (and possibly others)

Would you be open to adding support for the library to be loaded in environments that don't have a window global? I'd be happy to put together a PR.

[ericelliott]

Yep. I can't use this unless this is fixed. All my apps are universal JS. 👍

[rodneyrehm]

First up, thanks for this library :)

First up, thanks for trying to use it :)

Would you be open to adding support for the library to be loaded in environments that don't have a window global?

Of course the answer is yes!

The problem is: I don't have such an environment to test that kind of support. Also I have zero experience with "universal JS". @darobin has expressed the same requirement a couple of weeks ago and I was waiting for him to see what we could do about this.

That said, I have no idea what ally.js could possibly do without access to a browser's DOM and BOM. So what do you expect the non-browser environment to achieve with ally.js?

How would we go about testing this to avoid regressions? Can we do that in the browser, or do we have to run a second set of tests in Node?

I'd be happy to put together a PR.

I'd love that! How can I help?

[darobin]

A lot of the time you don't need the library to do anything: you just need it to load without crashing. That pretty much means not touching window or document.

If there is nothing useful for your library to do when loaded server-side (which I believe is largely the case) then all you need to do is short-circuit any code that runs when the library is loaded. A check of the kind if (typeof window !== 'undefined') around it ought to be enough.

[rodneyrehm]

A lot of the time you don't need the library to do anything: you just need it to load without crashing.

So what you need is the structure (.e.g. ally.foo.bar actually being a function), but if executed (what likely doesn't happen) it's effectively a dud (no operation performed)?

Are there any features that should run server-side?

A check of the kind if (typeof window !== 'undefined') around it ought to be enough.

That sounds simple enough.

[darobin]

Yeah pretty much.

To give you an example, here is how the last app I wrote works. It's all JS and React, all of the data fetching, rendering, etc is written as a client-side app. But it would be a shame if it stayed only that way since it's pretty much just textual articles. So on the server side I load it up and run the data fetching, templating, etc. so I get to send a pre-rendered article to the client (and React is smart enough to detect that is rendered the HTML and doesn't try to re-render). Some stuff that only makes sense on the client (e.g. tethering bits of UI with Tether.js) only runs on the client, but since it's the same code that gets loaded on the server, it just has to not crash.

Does that give you enough context to make this more meaningful?

[rodneyrehm]

Does that give you enough context to make this more meaningful?

yes. What is expected of ally.js is to successfully do exactly nothing when not run in a browser. Adding the following snippet to every module's default export should do the trick:

if (typeof window === 'undefined') {
  return undefined;
}

It would have been nice if we only had to do that in src/ally.js, but then you'd be barred from consuming modules (as opposed to the UMD bundle). We'll have this in version 1.1.0. I'm adding the "good first contribution" tag as this seems rather straight forward :)

[rodneyrehm]

Since window and document are not available in Web Workers as well, we can use those to run the tests. I could not find any reference to importScripts in the dojo loader (used by Intern), but RequireJS has got us covered there.

[rodneyrehm]

I've had another look at "invoking functions in non-browser env" and decided that it makes no sense to touch them. All of them need the DOM and some of them even verify arguments (explicitly requiring Element). So the first attempt at fixing this problem will only cover actually loading the library.

[rodneyrehm]

@JedWatson, @ericelliott, @darobin: please have a look at 1.1.0-beta.2 and tell me if your problem is resolved :)

[JedWatson]

Thanks @rodneyrehm - I've added that to Elemental, we'll let you know how it goes!

[ericelliott]

How did this shake out?

[rodneyrehm]

In elementalui/elemental#119 @JedWatson said he disabled the integration for now…

[ericelliott]

Thanks @rodneyrehm