Shims not being shimmed within node.js #703

maghoff · 2013-04-08T14:37:13Z

Consider the following files for a demonstration of code that works in the browser but fails in node:

shim-module.js:

console.log("Executing shim-module.js");
var x = "Loaded OK";

// Uncomment the following line to make this module act as an AMD module:
//define([], x);

index.js (node.js entry point):

#!/usr/bin/env node

var requirejs = require("requirejs");

requirejs.config({
    shim: {
        "shim-module": { "exports": "x" }
    },
    nodeRequire: require
});

requirejs(["shim-module"], function (x) {
    console.log(x);
});

index.html:

<!DOCTYPE html>
<html>
<head>
<script>
var requirejs = {
    shim: {
        "shim-module": { exports: "x" }
    }
};
</script>
<script src="node_modules/requirejs/require.js"></script>
<script>
require(["shim-module"], function (x) {
    console.log(x);
});
</script>
</head>
<body>
</body>
</html>

The problem seems to be that a dependency marked as a shim in node is never the less loaded as if it were an AMD module. This is verifiable by rewriting the module as an AMD and observing that it then works.

How to reproduce:

Store the files in a folder and npm install requirejs.
Open index.html in a web browser (I have been using Chrome) and inspect the log output. It says "Executing shim-module.js" and "Loaded OK".
Run "./index.js" on the command line and observe the output.

Expected: The node module should also output "Executing shim-module.js" and "Loaded OK".

Observed: Instead, it outputs "Executing shim-module.js" and "undefined", indicating that nothing has been exported from the shim module, even though it was executed.

The text was updated successfully, but these errors were encountered:

jrburke · 2013-04-09T01:31:55Z

As sorry for the confusion -- shim config is only meant for browser use. I cannot replicate it in node's environment, since each script is executed in its own sandbox, so things like var x are not visible outside the sandbox.

I will keep this ticket open to do a doc change to indicate that shim config is only for browser use.

maghoff · 2013-04-09T09:48:54Z

It would be helpful if the library would still resolve the shims and issue an error (throw an exception, maybe?) when the user is trying to load a shim from node. It is better for the user to have it fail properly rather than just failing silently like it does now :)

That being said, if you were to find a magical way of making the shims work in node as well, I would be grateful :)

jrburke · 2013-04-09T17:25:02Z

Good idea, I'll see about throwing in the node case to give an earlier clear error.

I do not see a way to get it to work though.

dcneiner · 2013-05-13T19:05:42Z

@jrburke I had shim config working fine in my node based unit tests, but now it breaks because you are forcibly throwing an error. Any chance you would consider reverting this change and making it documentation only… I see the predicament you are in (Should it fail silently, throw an error, or do something else). Anyway, I'll just set my npm version back to pre 2.1.6 for the time being.

dcneiner · 2013-05-13T19:07:34Z

@jrburke Let me clarify, my unit tests were running fine (vs. "shim config working fine") – I guess I worked around the shim issue, but now my unit tests are being killed by the error alerting me to the lack of shim support.

jrburke · 2013-05-14T04:25:10Z

I am definitely open to just making this maybe just a trace log statement and a doc blurb, but I would like to understand more how the unit tests run in such a way that they pass. As in, is it just a fluke or is it a setup others may use and therefore may hit this and limit their productivity.

Tentatively putting this in a 2.1.7 bucket, but would like to understand it more, since I obviously did not understand how shim might be used in this context well enough to make this change in the first place.

maghoff · 2013-05-14T10:02:05Z

For what it's worth, I think it is strange API design to make it do nothing when requiring modules that match configured shims. That's hardly an appropriate way to implement require for any other situation. Why should it be appropriate here?

When introducing better error reporting it shouldn't be entirely unexpected that somebody relies on former undocumented behaviour that was just there by accident. Common ways to handle this include introducing a configuration option to disable the new error reporting, or make it into an ugly warning. This way, projects that rely on the former, undocumented behaviour can get some leeway to adjust to the more sane way of handling this error situation.

To be clear: I think it is absolutely imperative for any API that it communicates clearly when it can not fulfill the contract for a given call.

dcneiner · 2013-05-17T04:43:57Z

@jrburke Sorry for the delay… it very well may be it worked for me because of how I was using Require for my tests. I'll try to make a reduced test case for you so you can see what I did.

dcneiner · 2013-05-29T18:22:17Z

@jrburke I worked on putting together a proof of concept, but while it worked for Underscore and Backbone, it failed with a custom file I tried to shim. What I eventually found out is that files that just declare var Global = ( ... module ... ) cannot be shimmed currently with RequireJS. However, files that use this syntax: this.Global = ( ... module ... ) work fine.

Please see my example project here: https://github.com/a2labs/require-shim-poc to see a working example.

I also tried hacking the r.js file to get the var Global = () syntax working. Inside your makeNodeWrapper I adjusted it to accept the shim config, and write to this.{shim.exports} = {shim.exports} if it wasn't already defined:

req.makeNodeWrapper = function (contents, shim) {
        return '(function (require, requirejs, define) { ' +
                contents +
                ( shim && shim.exports ? '\n;if( typeof this.' + shim.exports + ' === "undefined" ) this.' + shim.exports + " = " + shim.exports + ";" : '' ) + 
                '\n}(requirejsVars.require, requirejsVars.requirejs, requirejsVars.define));';
    };

(I pass in the shim in the req.load method) For this particular repo sample, this solution worked perfectly.

I see two options for shims in Node:

We pursue this idea (of automatically declaring this.varname = varname as part of the load if a shim is present) and see if it holds any water with a larger set of sample files.
Or, You update documentation to say: "In order for shims to work in Node.js, global variables to be shimmed must be declared using this.varname = ''; and not just a global var varname = '';"

I hope this helps. Let me know if you need me to do more digging for you.

giggio · 2013-05-30T14:46:31Z

Same problem here. I update to 2.1.6, and it broke my tests. I use the same requirejs config for the browser code, and for my tests, which run in node. And because shim config is not supported anymore I had to revert to 2.1.5.
You can see how this is working here:
https://github.com/giggio/open-store/blob/master/public/javascripts/test/support/runnerSetup.coffee
Notice the call to the 'boostrap' file and the end, which in turn has all the shims:
https://github.com/giggio/open-store/blob/master/public/javascripts/bootstrap.coffee
Shims seems to be working fine for me, even in node.

jrburke · 2013-06-10T16:39:50Z

@dcneiner thanks for diving into it. I am not comfortable just creating variables. I expect the this in your approach may not be valid if they get a "use strict" in there somehow. For shim config, there is a limit to the amount of magic I want to put in because it is a shim, and running shimmed browser code in Node is likely to have other problems too, besides the global use (use of document, navigator, etc...).

So what I am considering instead is just logging when shim config is in use in node and then log a message along the lines of "you may encounter errors, see [url] for more information" where [url] points to more info on the problem. Then removing the throw.

dcneiner · 2013-06-10T21:01:05Z

@jrburke I think that makes a lot of sense – it provides the message for those truly making a mistake, while not breaking it for those of us intentionally utilizing the feature.

gingi · 2013-06-25T22:07:42Z

I have a similar use-case as @dcneiner and @giggio, using the same browser-side configuration for Node-side unit tests (and nothing else). Is unit-testing considered a reasonable scenario for allowing shims in Node? Are there any other scenarios? Would it make sense to allow some "test mode" that supports Node-side shims. Even logging a warning might be overkill for a project with many unit tests.

Alternatively, an example of how to test browser-side shimful configurations in Node/headless-mode — without having to maintain a modified configuration — would be really helpful.

jrburke · 2013-06-27T00:35:54Z

@gingi the best solutions are to use modular code instead of shimmed code, or use a real browser for tests, like running them in phantomjs, or some web browser driver. The node env is not the browser env, so there is only so much that can be done to make it look like one. For me, it would be a mistake to go too far with it, as node is really just focused on server developer needs, not browser developer needs.

When I put in the log message, I will look at a way to disable it maybe via a config setting, but nothing promised until I have some time to really get into the code.

jrburke · 2013-07-07T04:42:57Z

Fixed in requirejs/r.js@55507ab

Now it just warns for each module that is loaded that has a shim config, and the warning can be suppressed via:

requirejs.config({
  suppress: {
    nodeShim: true
  }
});

giggio · 2013-07-07T13:44:58Z

@jrburke
Thanks for the fix!
The link for the commit is broken...
When should we expect a release and in which version?

jrburke · 2013-07-08T06:01:44Z

Fixed the URL, it had a trailing colon. 2.1.7 is released now, and it has this fix.

dcneiner · 2013-07-08T16:01:35Z

2.1.7 works like a charm, thanks for all your work on this @jrburke – really appreciate it!

giggio · 2013-07-08T21:34:38Z

Great! Just updated my code and it works! Thanks again!

eigood · 2014-08-18T06:18:49Z

The following works in node, where requirejs is an instance of the browser-side require library, and require is standard node. Modifying it to be dynamic based on the shim config shouldn't be all that hard.

The raw text is passed to Function(), to get around 'use strict' in the outer context. box2dweb has 'var Box2D' at the top, and doesn't use the window or this method.

function evalLoader(name, file) {
var rawText = require('fs').readFileSync(file).toString();
return Function(rawText + ';return ' + name + ';')();
}

requirejs.define('box2dweb', evalLoader('Box2D', 'bower_components/box2dweb/Box2dWeb-2.1.a.3.js'));

jrburke closed this as completed in requirejs/r.js@28bba6d May 9, 2013

jrburke reopened this May 14, 2013

jrburke added a commit that referenced this issue Jul 7, 2013

Refs #703, mention problem using shim config in node

fb2cdf4

jrburke closed this as completed Jul 7, 2013

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Shims not being shimmed within node.js #703

Shims not being shimmed within node.js #703

maghoff commented Apr 8, 2013

jrburke commented Apr 9, 2013

maghoff commented Apr 9, 2013

jrburke commented Apr 9, 2013

dcneiner commented May 13, 2013

dcneiner commented May 13, 2013

jrburke commented May 14, 2013

maghoff commented May 14, 2013

dcneiner commented May 17, 2013

dcneiner commented May 29, 2013

giggio commented May 30, 2013

jrburke commented Jun 10, 2013

dcneiner commented Jun 10, 2013

gingi commented Jun 25, 2013

jrburke commented Jun 27, 2013

jrburke commented Jul 7, 2013

giggio commented Jul 7, 2013

jrburke commented Jul 8, 2013

dcneiner commented Jul 8, 2013

giggio commented Jul 8, 2013

eigood commented Aug 18, 2014

Shims not being shimmed within node.js #703

Shims not being shimmed within node.js #703

Comments

maghoff commented Apr 8, 2013

jrburke commented Apr 9, 2013

maghoff commented Apr 9, 2013

jrburke commented Apr 9, 2013

dcneiner commented May 13, 2013

dcneiner commented May 13, 2013

jrburke commented May 14, 2013

maghoff commented May 14, 2013

dcneiner commented May 17, 2013

dcneiner commented May 29, 2013

giggio commented May 30, 2013

jrburke commented Jun 10, 2013

dcneiner commented Jun 10, 2013

gingi commented Jun 25, 2013

jrburke commented Jun 27, 2013

jrburke commented Jul 7, 2013

giggio commented Jul 7, 2013

jrburke commented Jul 8, 2013

dcneiner commented Jul 8, 2013

giggio commented Jul 8, 2013

eigood commented Aug 18, 2014