mozilla / nunjucks

A powerful templating engine with inheritance, asynchronous control, and more (jinja2 inspired)
https://mozilla.github.io/nunjucks/
BSD 2-Clause "Simplified" License
8.55k stars 638 forks source link

require('nunjucks') within webpack returns an empty object #507

Open at0g opened 9 years ago

at0g commented 9 years ago

When requiring nunjucks within a webpack project, an empty object is returned instead of nunjucks.

eg.

// webpack.config.js
module.exports = {
    entry: './main.js'
};
// main.js
var nunjucks = require('nunjucks');
console.log(nunjucks); // outputs {}
at0g commented 9 years ago

The config file below is a workaround - it borrows the plugins configuration from nunjucks/bin/bundle.js to create a non-slim web build.

// webpack.config.js
var webpack = require('webpack');
module.exports = {

    entry: './main.js',

    plugins: [
        new webpack.NormalModuleReplacementPlugin(/nunjucks$/,
            'nunjucks/index.js'),

        new webpack.NormalModuleReplacementPlugin(/(path|fs)$/,
            'node-libs-browser/mock/empty'),

        new webpack.NormalModuleReplacementPlugin(/nunjucks.+precompile$/,
            'node-libs-browser/mock/empty'),

        new webpack.DefinePlugin({
            'process.env': {
                IS_BROWSER: true
            }
        }),
        new webpack.NormalModuleReplacementPlugin(/nunjucks.+loaders\.js$/,
            './web-loaders.js')
    ]
};
jgerigmeyer commented 9 years ago

Like many bundlers, Webpack uses the browser field from package.json for the entry point, so in this case it's pulling in https://github.com/mozilla/nunjucks/blob/master/browser/nunjucks.js. That file doesn't export any modules, but simply defines the local nunjucks variable. Thankfully, Webpack has a loader specifically for this case: https://github.com/webpack/exports-loader

In your webpack.config.js file:

module.exports = {
  ...
  module: {
    loaders: [
      {
        test: /nunjucks\/browser\/nunjucks\.js$/,
        loader: 'exports?nunjucks'
      }
    ]
  }
}

This will expose the nunjucks local variable from the browser/nunjucks.js file whenever it's required.

We could modify nunjucks to add a UMD wrapper or export a variable, but the current approach seems the simplest and most flexible with various bundlers and browser environments.

carljm commented 9 years ago

@jgerigmeyer Do you think that's worth adding to the docs somewhere?

at0g commented 9 years ago

Thanks for the response @jgerigmeyer - using exports loader works well for the browser, but webpack cant compile the chokidar dependencies if the bundle targets node... not really a game breaker but it would be great to be able to publish to both targets.

davidchase commented 9 years ago

@carljm i think it should be documented, because it also seems to be doing the same thing with browserify. Anyone coming to nunjucks for the 1st time doesn't expect this resolution as the default just my $0.02.

to do it within browserify you need to shim it like webpack using browserify-shim transform.

adding something like this to package.json:

  "browserify": {
    "transform": [
      "browserify-shim"
    ]
  },
  "browserify-shim": {
    "nunjucks": "nunjucks"
  }
jgerigmeyer commented 9 years ago

I agree that it should at least be better-documented, and possibly also wrapped in some sort of UMD wrapper if most people want to use it that way.

@at0g Do you have a suggestion for a better way to ignore chokidar when targeting node? Or just documenting that better too?

at0g commented 9 years ago

@jgerigmeyer I think having a slim version (for node) that drops the watch/compile functionality would be a good way to get around chokidar. My use case was creating a node bundle where everything is precompiled anyway, making the watch functionality redundant, so a node-slim version would be perfect.

Would you be open to a PR for this?

jgerigmeyer commented 9 years ago

I think that'd be reasonable. @carljm what do you think?

carljm commented 9 years ago

Seems fine to me, sure.

Rowno commented 8 years ago

I just ran into this bug with Browserify too, had me stumped for a couple of hours.

Rowno commented 8 years ago

FYI, it works fine in v1.3.4.

unlucio commented 8 years ago

I bumped my head trying to have it compile in my app.js file with browserify with no success and I had to roll back to 1.3.4 :/ The browserify-shim solution didn't make it for me. Any suggestion?

carljm commented 8 years ago

556 is another report of this problem, with a PR to output a CommonJS2 module.

timkelty commented 8 years ago

Another option for webpack is to use the nunjucks loader: https://github.com/at0g/nunjucks-loader

xogeny commented 8 years ago

I just ran into this issue today as well. I just rolled back to 1.3.4. This was simpler than complicating my webpack.config.js file (at least for now).

carljm commented 8 years ago

600 is another issue asking for a way to build without chokidar

jgerigmeyer commented 8 years ago

Fixed by https://github.com/mozilla/nunjucks/pull/606.

carljm commented 8 years ago

So after some further conversation here, I think the root problem is our use of the browser field in package.json. The browser field is intended to give browser-builders (webpack, browserify) a module-based entry point which replaces or removes any non-browser-compatible code (in our case, just the precompiler?). Pointing it to an actual browser build (which just sets the nunjucks variable and doesn't export anything) is just wrong. Turning that browser build into a UMD module is a workaround, it's not fixing the real problem.

The right solution, I think, is to provide a dedicated entry point for webpack/browserify which doesn't require the precompiler, but otherwise still uses the source modules, and point our browser field to that instead. (And then we may want a second entry point for a slim build, which doesn't require any of the stuff you don't need in a slim build -- the use of this entry point would need to be specifically documented, I think, since we wouldn't want to make it the default in the browser field.)

Alternatively, it might be possible to make use of the object syntax for the browser field to tell webpack/browserify to leave out the precompiler module? Then maybe we only need an alternate entry point for slim builds. AFAICT this is the canonical documentation for the browser field: https://gist.github.com/defunctzombie/4339901 -- and here's the library for interpreting it, which might be useful in testing our setup: https://www.npmjs.com/package/browser-resolve

The above is all "my best understanding at the moment, pending an actual attempt to implement." I'll try to get to this if I can find some time, but pull requests would be very welcome.

jgerigmeyer commented 8 years ago

:+1:

AkeemMcLennon commented 8 years ago

:+1:

nachbarshund commented 8 years ago

Is there a solution for using Grunt?

daslicht commented 8 years ago

related: https://github.com/thlorenz/browserify-shim/issues/207 https://github.com/rotundasoftware/nunjucksify/issues/16

dgbeck commented 8 years ago

Hi @carljm thanks for your attention to this issue.

I'm still somewhat confused but would really like to help solve this problem.

To me there are two separate but related issues in this thread.

1) The inability to effectively use the full version of nunjucks from browserify / webpack environments. 2) The inability to effectively use nunjucks-slim from those environments.

The right solution, I think, is to provide a dedicated entry point for webpack/browserify which doesn't require the precompiler, but otherwise still uses the source modules, and point our browser field to that instead. (And then we may want a second entry point for a slim build, which doesn't require any of the stuff you don't need in a slim build -- the use of this entry point would need to be specifically documented, I think, since we wouldn't want to make it the default in the browser field.)

The second entry point that is being described.. is this not what browser/nunjucks-slim.js already is intended to be? If that is the case, then all that is missing is a UMD wrapper around browser/nunjucks-slim.js to solve (2). Can I submit a PR to be reviewed for just this case? Or is there some alternative to the UMD wrapper that is preferred?

I am not in a great position to solve (1), since I'm unsure how to strip out the precompiler, and we don't need this use case, but solving (2) seems straight forward and quite tangential.

Thank you!

carljm commented 8 years ago

@dgbeck Thanks for the thoughts.

Effectively you're re-hashing the same discussion from above, except for the "slim" case specifically. But the slim case is actually no different from the non-slim case, and all the same discussion from above still applies. No, browser/nunjucks-slim.js is not intended to be a slim entry point for webpack/browserify. It is the output from nunjucks' default webpack build, and it is intended for use directly in the browser.

Unfortunately we currently lack proper module entry points for webpack / browserify. It is possible to hack around that by using the files in browser/ in your own webpack/browserify build, but you have to do some extra work to get at the nunjucks var, since it isn't exposed via UMD/whatever. How to do that "extra work" for webpack is documented above.

The first fix for this problem that was proposed above is to just make the browser/ files all export via UMD. That's certainly simple, but I think it's wrong -- it's silly to add UMD cruft to files that are intended for browser use. We already have the source modules, and webpack / browserify should use those, not the browser output files.

What we need here is a PR to provide proper module entry points for non-slim and slim. Both need to strip out the precompiler; slim needs to strip out some other things in addition.

Basically I'm just repeating what I already said in https://github.com/mozilla/nunjucks/issues/507#issuecomment-164545200

dgbeck commented 8 years ago

Ah, now I think I get it. Thanks for the added explanation. The difference is that the approach you are describing would directly load the source files in src, instead of adding a UMD wrapper to the output bundle built with /bin/bundle.js and then loading the wrapped bundle.

I get the desire for purity but it does feel kinda like a longer road to the same destination.

Also, has the AMD case been considered? Seems like in oder to support nunjucks-slim in an AMD environment some sort of wrapper will still be required, either around the src files themselves or around the bundle. Is there a desire to support AMD and if so does that impact this issue?

carljm commented 8 years ago

Yes, that's right.

If it turns out to be hard to do, or has other disadvantages, we can decide to do the UMD wrapper in the browser output files instead. But at the moment I don't think anyone has actually dug in and tried to do it yet, so we don't know if it's a "longer road" or not.

I'm not real familiar with AMD, but I think AMD loaders generally have techniques for shimming non-AMD code, so I don't know that explicit AMD support is a high priority.

dgbeck commented 8 years ago

Ok, makes sense. Thanks.

I would like to help resolve this issue asap. nunjucks is too good of a template engine to suffer from such a basic problem.

I just tried my hand at making a slim common-js entry point to explore that approach further. I took a look at what is stubbed out by the bin/bundle.js script when run with the slim option, and then created a new file nunjucks-commonjs-slim.js in the browser directory, which is very much like index.js, except without any references to the modules that were stubbed out. The resulting file clearly duplicates some code that is in index.js, and semantics that are in bin/bundle.js. I don't think there is a way around this duplication since we are basically creating a new parallel way to do the same thing as those two files.

'use strict';

var lib = require('../src/lib');
var env = require('../src/environment');
var Loader = require('../src/loader');

module.exports = {};
module.exports.Environment = env.Environment;
module.exports.Template = env.Template;

module.exports.Loader = Loader;
module.exports.FileSystemLoader = loaders.FileSystemLoader;
module.exports.PrecompiledLoader = loaders.PrecompiledLoader;
module.exports.WebLoader = loaders.WebLoader;

module.exports.runtime = require('../src/runtime');
module.exports.lib = lib;

module.exports.installJinjaCompat = require('../src/jinja-compat.js');

// A single instance of an environment, since this is so commonly used

var e;
module.exports.configure = function(templatesPath, opts) {
    opts = opts || {};
    if(lib.isObject(templatesPath)) {
        opts = templatesPath;
        templatesPath = null;
    }

    var TemplateLoader;

    e = new env.Environment(TemplateLoader, opts);

    if(opts && opts.express) {
        e.express(opts.express);
    }

    return e;
};

module.exports.compile = function(src, env, path, eagerCompile) {
    if(!e) {
        module.exports.configure();
    }
    return new module.exports.Template(src, env, path, eagerCompile);
};

module.exports.render = function(name, ctx, cb) {
    if(!e) {
        module.exports.configure();
    }

    return e.render(name, ctx, cb);
};

module.exports.renderString = function(src, ctx, cb) {
    if(!e) {
        module.exports.configure();
    }

    return e.renderString(src, ctx, cb);
};

However requiring this file from the browser causes an error because /src/environment requires src/loaders, which requires src/node-loaders, which is clearly not intended for the browser, and which requires chokidar.

Since rewiring the dependencies is not possible without hacking the browserify / webpack config, the two options I see are to:

1) get rid the the watch / chokidar functionality in the file system loader in the default configuration, requiring people to configure that functionality if they want it. or

2) inject / add the file system loader to the Environment object from index.js. This would necessitate a breaking API change since it is the Environment object is quite public and right now it comes with that loader out of the box.

Both (1) and (2) amount to dropping the watch functionality from the built in File System loader (or eliminating the built in status of the loader completely), relying on the user to add it him or herself.

I stopped here as I feel like both of these are not great options and I was about to enter the rabbit hole, but I can keep going if you think it is worthwhile.

I just took a look at the UMD route. Looks like there is a convenient webpack option to export to umd, that fits right into the current webpack build mechanism. Works great in our environment. Tests passing. It is a one-liner. The PR is here. What do you think?

Let's get this behind us! Thank you!!

carljm commented 8 years ago

@dgbeck Thanks for your exploration. I think the problems you ran into are solvable (for instance, the removal of chokidar and the watch option for 3.0 is already planned -- see #610). However, since I'm not sure when I'll be able to look into it, and nobody else has stepped up so far, I'll merge your PR for now as a stopgap solution, at least, for remaining releases in the 2.x series. If we're able to get the purer solution working well, that'll be a change in 3.0.

Thanks again!

dgbeck commented 8 years ago

Gotcha. Removing chokidar seems like a solid idea!

Thanks for merging this for now, and also for your help maintain this awesome library. It is so slick in so many ways!

daslicht commented 7 years ago

any news on this ?

vecmezoni commented 7 years ago

It should be fixed in 3.0 with https://github.com/mozilla/nunjucks/pull/606