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

Use with? #1

Closed
wesleytodd opened this issue Jan 7, 2018 · 8 comments
Closed

Use with? #1

wesleytodd opened this issue Jan 7, 2018 · 8 comments

Comments

@wesleytodd
Copy link

Typically I wouldn't ever recommend using with, but in this case it seems like it would improve the api of this module to switch to something like this:

module.exports = (templateStr) => return new Function('d', 'with (d) { return `' + templateStr + '` }');

The performance hit is pretty minimal (see below), and it does not force users to have that d. strewn through their templates. What do you think? Or is the performance aspect the main focus of this package?

Here is the perf test I did:

$ cat template-literal.js
var tmplWith = (str) => new Function('d', 'with (d) { return `' + str + '` }');
var tmplWithout = (str) => return new Function('d', 'return `' + str + '`');

var time = (fnc) => {
  console.time(fnc.name);
  for (let i = 0; i < 10000; i++) fnc.call(null, i);
  console.timeEnd(fnc.name);
};
time(function runWith (i) {
	tmplWith('i: ${i}')({ i });
});
time(function runWithout (i) {
	tmplWithout('i: ${d.i}')({ i });
});

$ node template-literal.js
node template-literal.js
runWith: 16.748ms
runWithout: 11.198ms
@Drulac
Copy link
Owner

Drulac commented Jan 7, 2018

That have a little performance impact on compilation, but the impact is huge on running :

With : Literal render x 455,629 ops/sec ±0.20% (84 runs sampled)
Without : Literal render x 3,278,679 ops/sec ±0.44% (81 runs sampled) 

It became 7 times more long... It's a good improvement, I think the performance loss is to high to add it.

(In the bench/ directory there is compile.js who is perfect to benchmark, you can comment other frameworks' tests to win time)

@Drulac Drulac closed this as completed Jan 7, 2018
@wesleytodd
Copy link
Author

Ok, so performance is your biggest concern for this module then. That is why I asked, because it is obviously slower to use with. My very minimal test did not show such poor performance as yours did or I wouldn't have even brought it up. The performance hit probably scales with the number of variable lookup's.

Anyway, I just thought it was worth bringing up because the hard coded d variable for access inside the templates seemed less user friendly than the with version.

@Drulac
Copy link
Owner

Drulac commented Jan 7, 2018 via email

@Drulac
Copy link
Owner

Drulac commented Jan 7, 2018 via email

@wesleytodd
Copy link
Author

I tested compilation and rendering in one go. I agree that is not technically accurate to a use case, it was just my 2 second look at what the perf difference would be. I probably wont be using the module anyway, just thought I would give my feedback since you posted over on the express repo. But I do think that adding the option to change the variable name might help with other people wanting to use this, again just my 2c.

@Drulac
Copy link
Owner

Drulac commented Jan 7, 2018

Thanks for you feedback :-)

I'm french, and not really good at english, I havn't understand "again just my 2c" 😅

@wesleytodd
Copy link
Author

Haha, no worries. "just my 2c" is an English saying which means "I am giving my personal opinion". See https://en.wikipedia.org/wiki/My_two_cents

@Drulac
Copy link
Owner

Drulac commented Jan 7, 2018 via email

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

2 participants