Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Imports choking esbuild --bundle #341

Open
cfv1984 opened this issue Feb 18, 2022 · 6 comments
Open

Imports choking esbuild --bundle #341

cfv1984 opened this issue Feb 18, 2022 · 6 comments

Comments

@cfv1984
Copy link

cfv1984 commented Feb 18, 2022

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 evanw/esbuild#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
Copy link

evanw commented Feb 18, 2022

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
Copy link
Collaborator

PR welcome

@jacargentina
Copy link

@niftylettuce please i made the PR ? Thanks

@jacargentina
Copy link

@niftylettuce any chance to merge my fix?

@titanism
Copy link

titanism commented Jun 8, 2023

@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 🙏

Screen Shot 2023-06-08 at 3 05 12 PM

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

titanism added a commit to ladjs/consolidate that referenced this issue Jun 9, 2023
…or API changes in v8, removed support for razor-tmpl removed marko support (per <fastify/point-of-view#218>), fixed try/catch issues (per <tj#341>)
@titanism
Copy link

titanism commented Jun 9, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants