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

Process doesn't exit after renderString() #369

Closed
subfuzion opened this issue Feb 8, 2015 · 24 comments
Closed

Process doesn't exit after renderString() #369

subfuzion opened this issue Feb 8, 2015 · 24 comments

Comments

@subfuzion
Copy link

Demonstrated with simple example:

var nunjucks = require('nunjucks');

var template = 'Hello {{ name }}';
var data = { name: 'World' };

// This will also prevent process from exiting:
//var result = nunjucks.renderString(template, data);
//console.log(result);

nunjucks.renderString(template, data, function(err, result) {
  console.log(result);
});

// process doesn't exit after renderString
@mihalik
Copy link

mihalik commented Feb 9, 2015

Possibly related, I use Nunjucks with Metalsmith using metalsmith-templates and the metalsmith process hangs with Nunjucks 1.2.0 and completes when I downgraded back to 1.1.0.

@jlongster
Copy link
Contributor

@tonypujals nunjucks turns on watching by default (this may change) but you just need to call nunjucks.configure({ watch: false })

@mihalik that's surprising. I'll have to take a look at that and see if we changed that code at all.

@mihalik
Copy link

mihalik commented Feb 9, 2015

I spent a while digging through Metalsmith code today before I tried dropping back to 1.1.0. I just tested 1.2.0 and had it hang and dropped back to 1.1.0 again and it is not hanging. I can file a separate ticket if you would like.

@mihalik
Copy link

mihalik commented Feb 9, 2015

Felt bad about just saying it is broken without providing a test. So I made a pared-down example.

@jlongster
Copy link
Contributor

Wonderful, thanks! I will test it out tomorrow. We can continue in this ticket.

@subfuzion
Copy link
Author

@jlongster, good to know, thanks. I published a nunjucks utility on npm that currently call process.exit explicitly, but I'll try the suggestion out.

https://github.com/tonypujals/njx

@subfuzion
Copy link
Author

@jlongster your suggestion resolves the issue for me. Unless you want to keep it open to investigate @mihalik's example, please feel free to close it. Thanks. 👍

@subfuzion
Copy link
Author

fwiw, i do think the default option for watch should be false

@nfroidure
Copy link

👍 for watch beeing false per default, lost an hour to find out that Nunjuncks was responsible...

@jlongster
Copy link
Contributor

I'll think about turning watch to default for 2.0. @garygreen @carljm @SamyPesse what do you think?

@subfuzion
Copy link
Author

I definitely think false should be the default. I think it's unexpected behavior for enough people that it should be opt in.

Having to do this just doesn't feel right.

@carljm
Copy link
Contributor

carljm commented Apr 29, 2015

Yes, I'm surprised it defaults to true.

@garygreen
Copy link
Contributor

Agree, watch should be off by default.

@jlongster
Copy link
Contributor

Ok, sounds good, done. Now we just need to decide whether autoescaping should be on by default... there is a bit of a perf hit

@jlongster
Copy link
Contributor

Obviously this will go out with 2.0, I have several things on master so might as well just queue up for 2.0 now. I think I'll push it out after fixing just a few more bugs lists in the 2.0 issue

@carljm
Copy link
Contributor

carljm commented Apr 29, 2015

I'd be in favor of autoescape by default if we're making a breaking release. I think it's pretty clear that it's the right setting for most uses of nunjucks. It's not hard to turn off in those unusual cases where you don't need it and/or need that extra little bit of performance.

@jlongster
Copy link
Contributor

I think you're right. The majority of people doing real websites need to turn it on anyway.

@popomore
Copy link
Contributor

I think we should create a new branch for 2.0, and master will publish quickly when compatible feature or patch merged. People need some fix as soon as posible without waiting 2.0 that maybe contain some bugs from new feature.

@jlongster
Copy link
Contributor

@popomore I dislike forks when we can avoid it, and I think it's best to move forward as much as possible rather than create multiple "trains". It's unavoidable if there's a large rewrite, but 2.0 is not going to be that. I think I'm actually pretty close to finishing 2.0. There's just one or two features listed in the 2.0 issue that I'd like to implement, and then I'm going to release it.

We will have to delay the release a little bit more than usual, yes, but I estimate I can push out 2.0 next week or the week after.

@popomore
Copy link
Contributor

Got it.

@jlongster jlongster mentioned this issue Apr 29, 2015
@subfuzion
Copy link
Author

Thanks! 👍

@subfuzion
Copy link
Author

With regard to autoescape, I may be in the minority with respect to nunjucks use cases for the primary developer audience, but I definitely prefer opt in over magic (especially when it also impacts performance). Missing options should mean default everything to false; provide options to turn things on. I don't feel especially religious about this ... just my $0.02. It's not a major burden to add the option to disable, just doesn't feel right to me, and I use nunjucks for more than just website code.

@carljm
Copy link
Contributor

carljm commented Apr 29, 2015

@tonypujals I sympathize with that view in general, but I think it's trumped by the importance of "default to safe" when a security-sensitive parameter is involved.

@nfroidure
Copy link

I agree with @carljm, always default to security.

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

7 participants