-
Notifications
You must be signed in to change notification settings - Fork 643
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
Comments
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. |
@tonypujals nunjucks turns on watching by default (this may change) but you just need to call @mihalik that's surprising. I'll have to take a look at that and see if we changed that code at all. |
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. |
Felt bad about just saying it is broken without providing a test. So I made a pared-down example. |
Wonderful, thanks! I will test it out tomorrow. We can continue in this ticket. |
@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. |
@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. 👍 |
fwiw, i do think the default option for watch should be |
👍 for watch beeing false per default, lost an hour to find out that Nunjuncks was responsible... |
I'll think about turning watch to default for 2.0. @garygreen @carljm @SamyPesse what do you think? |
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. |
Yes, I'm surprised it defaults to true. |
Agree, watch should be off by default. |
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 |
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 |
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. |
I think you're right. The majority of people doing real websites need to turn it on anyway. |
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. |
@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. |
Got it. |
Thanks! 👍 |
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. |
@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. |
I agree with @carljm, always default to security. |
Demonstrated with simple example:
The text was updated successfully, but these errors were encountered: