Imports choking esbuild --bundle

tj / consolidate.js

Template engine consolidation library for node.js

3.48k stars 357 forks source link

Imports choking esbuild --bundle #341

Open cfv1984 opened 2 years ago

cfv1984 commented 2 years ago

Hi! For a toy project of mine I used koa-views, which uses consolidate, and noticed that the way consolidate does requires chokes up esbuild --bundle, causing me to have to manually mark every single template engine I'm not using as external, which in esbuild means it won't care for them.

This however is quite a chore and if there's ever a change in the supported engines will mean I'll have to update this list by hand which is ... not ideal.

One way I thought could potentially work -and will likely test for viability shortly- is abstracting the require calls such that the bundling tools no longer get confused about all those conditional calls?

I created https://github.com/evanw/esbuild/issues/2033 over at esbuild so someone can at some point have a shot at solving it, but also wanted to let y'all know this is a thing that happens.

Cheers!

evanw commented 2 years ago

An easy way to avoid this problem with esbuild is to move each conditional require() call inside the try/catch statement:

 exports.velocityjs.render = function(str, options, cb) {
   return promisify(cb, function(cb) {
-    var engine = requires.velocityjs || (requires.velocityjs = require('velocityjs'));
     try {
+      var engine = requires.velocityjs || (requires.velocityjs = require('velocityjs'));
       options.locals = options;
       cb(null, engine.render(str, options).trimLeft());
     } catch (err) {
       cb(err);
     }
   });
 };

This signals to esbuild that the require() call is expected to be able to fail and the failure should be handled at run-time instead of at compile-time.

niftylettuce commented 2 years ago

PR welcome

jacargentina commented 2 years ago

@niftylettuce please i made the PR ? Thanks

jacargentina commented 1 year ago

@niftylettuce any chance to merge my fix?

titanism commented 1 year ago

@jacargentina we are merging your fix into our new repo 🙏

We have forked this repository for maintenance and released it under @ladjs/consolidate, see https://github.com/ladjs/consolidate.js. We have merged PR's and updated it for email-templates. Please click the "Watch" button to get notified of all releases at https://github.com/ladjs/consolidate.js. Thank you 🙏

PR welcome at the new repo once new release is published today!

titanism commented 1 year ago

v1.0.0 of consolidate has been released (and mirrored to @ladjs/consolidate) 🎉

watch/follow https://github.com/ladjs/consolidate for updates and to submit future issues and PR's