window detection forces pre-compilation of paper-node

[georeith]

So it seems that recently you have removed the paper-node.js build file and replaced it with automatic detection of whether the window exists. (on prebuilt/module at least, not sure if real releases will be different?)

The issue is that a lot of us have pre-compilation and anyone using some kind of module system with node that imports paper via NPM will find they are being forced to import paper node.

I would argue that it was better having the separate distributions before. It was trivial for me to tell my module loader to import different versions for different environments (test, development, production), whereas now I have no control over that and it actually breaks on compilation as jsdom is not compatible with webpack.

[lehni]

The reason for this change was that many people wanted to use NPM to install paper.js for browser-sided use, and were asking for a hybrid library (see #739). The merge to a unified code-base with no pre-processing branching has greatly simplified development and is now also my preferred way of developing paper.js

I don't fully understand the issue at hand (not a webpack user myself), so here a couple of questions:

• Does webpack run the code that it packs in order to detect dependencies? If so, that surely must create issues with many libraries that do feature detection in such a way?
• What does "precompilation" mean in this context? JS is not a compiled language.
• Why can't webpack just pack the library and assume it's for the browser?

@louisremi you seem to have managed to overcome this. Could you help us out here?

[georeith]

@lehni PaperJS gets built into our source code and never hits the global scope because we require/import it in our files. This then gets stitched up by webpack, basically its the same as require() in node except an intermediary stitches these files together. The issue is that this runs in node where there is no window and it always imports paper-node.

On the flip side for my tests I do want to load paper-node when I require/import it as these run in a browserless context.

[lehni]

It seems there is no way to make it easy for all the existing scenarios out there. Things seem to be in a state of mess, with way too many methods of requiring / bundling JS project files. Of course we have to solve this, but I don't want to go back to separate files for browser / node, because that too has caused way too many issues in the past.

[lehni]

@georeith when you say "it always imports paper-node", you mean it always requires ./node/window? There is no paper-node.js anymore.

[georeith]

@lehni yeah it attempts to build that into it also.

So is it possible we could use node environment variables and fallback on window detection. E.g., if process.env.PAPER_TARGET === 'browser'. Not the nicest for sure and you can probably think of some better naming, but at least it gives you a way to require the core version in node.

[lehni]

I'd like to wait for @louisremi and hear how he solved it first...

[lehni]

@georeith could you provide me with a simple test-case, a set of configuration files for me to repeat the problem locally?

[georeith]

@lehni https://github.com/georeith/paperjs-issues-986-example

You may need to install webpack globally not sure. The build fails but thats not the real issue, the real issue is that I can't tell it I don't want to inclune ./node/* at the moment.

[georeith]

@lehni Added browserify to the example aswell to show its not just webpack. I'm sure requireJS, .etc all have the same issue. Even babel and other ES2015 transpilers fail with import export syntax as that just gets rewritten to require().

[louisremi]

Hey, here's our webpack.config.js.
We made the ./node/window and ./node/extend modules external. This way they aren't included in the bundle, and the require() code should never be executed in the browser.

[georeith]

@louisremi Thanks for that.
Strangely though that doesn't work for me... results in the following:

function(module, exports) {
    module.exports = ./node/window;
 }

Which of course syntax errors.

[georeith]

@louisremi
In your library (looks really sweet btw) it becomes the following because you are exporting to UMD

    if(typeof exports === 'object' && typeof module === 'object')
        module.exports = factory(require("./node/window"), require("./node/extend"));
    else if(typeof define === 'function' && define.amd)
        define(["./node/window", "./node/extend"], factory);

Whilst thats a possibility it definitely doesn't look like intended behaviour for webpack. As they don't path.resolve it either before comparing.

[louisremi]

Whilst thats a possibility it definitely doesn't look like intended behaviour for webpack. As they don't path.resolve it either before comparing.

Not sure exactly what you mean, but my build file takes care of keeping those node scripts where they're supposed to be, in case Plumin is used in node.

At some earlier point I aliased those module paths to a noop module, which resulted in a dist file that didn't contain any require but couldn't be used in Node.

[georeith]

@louisremi What I mean is that works when you use UMD by accident (because it uses strings). It is not really what externals is for as you can see by the invalid syntax it generates without UMD export on.

@lehni So I've been thinking this would of been solved by the old version by using the package.json browser field spec which is supported by webpack, browserify and I assume most module builders.

I know you said you don't want to but whats wrong with having a separate version for node that looks like this:

require("./node/window");
var paper = require("./paper-full");
require("./node/extend")(paper);

You can then set package.json like this:

{
    "main": "./dist/paper-node.js",
    "browser": "./dist/paper-full.js"
}

Is there something I'm missing about this?

[louisremi]

Ok. I'm using UMD purposefully, are there good reasons for not using it?

[georeith]

@louisremi Not that I know of I will probably swap to it at least for now.

I just think we need a more concrete solution to this as the module style is pretty prevalent these days and don't think people should have to jump through hoops to get something like that working.

[lehni]

@georeith thanks for the test-case!
@louisremi thanks for sharing your configuration.

Is there a way to include a configuration file for web-pack to tell it what to ignore?

I really would like to find away to avoid using separate library files, if possible.

[lehni]

@georeith I'm just seeing your suggestion for node now. That's a good point, probably the way to go! I just need to find a way to pass the window object from require('./node/window'); on to the call of require('./paper-full'), without export things through the global object, which would be very un-node.js-like... Any suggestions?

If we go this route, then there will probably only be paper-full.js for node.js, which I think is OK (and was the case before).

[lehni]

@georeith to explain my last question a bit more: the way it is right now, require('./node/window'); returns the window object, which then gets assigned to self and window in Node. Removing this statement from paper-core.js will therefrore not work, we need to find a way to bring this window object into the library scope.

One way that we could achieve this is rather simple: self already gets passed on to the scope function of the library, but right now we always call this function. We could make a distinction there, and only call it if self is defined (browsers and workers), and return the function instead if self is not defined. This would then allow the paper-node.js shim to require the JSDom window and pass it on aser self... The problem here is that this would stop working if something else loaded in Node.js would expose something under global.self. This would be considered bad practise though...

I'm not super happy with this proposal but it's the only solution I can see. That, or we go back to building separate libraries, which I guess is also an option.

[lehni]

What we shouldn't forget is scenarios like these: #739 (comment)

This is the reason why I didn't want separate versions, and why I thought the current solution was a nice one. There just doesn't seem to be a way to tick all the boxes, which is really quite frustrating.

[lehni]

Another idea: We could feature-detect webpack, e.g. by checking for require.context(), and if it's present, not require window. This should work?

[lehni]

Feature-detection doesn't work because webpack isn't running the code, it's parsing it, looking for calls of require() and trying to make sense out of them. Say what you will about webpack, it feels like one massive hack to me that attempts to fix a problem that really should be addressed in the language instead : )

[lehni]

I believe I found a solution now, as outlined here:
https://github.com/defunctzombie/package-browser-field-spec#ignore-a-module

[georeith]

@lehni Apologies I didn't see your question yesterday when I read that. I assume you can't use module.exports?

It's not just webpack, its pretty much any bundler and there are many of them. Webpack just happens to be what I use.

What's the solution you found?

[lehni]

Solution coming up in 1s : )

[lehni]

Let me know if this works for you. It seems to do the trick just fine here.

[georeith]

Works great. Tried that in my package.json before but it didn't work. I guess it has to be within the module that requires stuff.