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

Non-global IIFE format for compile API #345

Closed
taylorzane opened this issue Mar 6, 2017 · 14 comments
Closed

Non-global IIFE format for compile API #345

taylorzane opened this issue Mar 6, 2017 · 14 comments

Comments

@taylorzane
Copy link
Contributor

taylorzane commented Mar 6, 2017

It would be nice to have a compiler format that simply outputs an IIFE that doesn't not assign the output to a global variable.

My reasoning behind this is such that I'd like to be able to compile a component in the browser, eval that code, and then assign the output to a state tree node, or a local variable.

Currently I'm using the IIFE format and just running replace on the output code before eval-ing the code.

Example:

const myTemplate = svelte.compile('my template code', {
  output: 'iife',
  name: 'myComponentName'
})

const code = myTemplate.code.replace('var myComponentName =', '')

const myComponent = eval(code)

new myComponent({
  target: document.querySelector('#div')
})

I would like to be able to do this, to avoid any possible collisions. It's possible that the variable I'm using as the IIFE name could be used (I understand that I could use some jargon, but I consider that a naïve practice)

Example:

const myTemplate = svelte.compile('my template code', {
  output: 'local-iife'
})

const code = myTemplate.code

const myComponent = eval(code)

new myComponent({
  target: document.querySelector('#div')
})

I'm willing to make this PR (it should be as simple as creating a new intro/outro), I just wanted to have an open discussion before spending any time on this.
My questions are as follows:

  1. Is this something the Svelte community would be interested in?
  2. What should the name of the format be?
    a. Or, should it still be iife, but with another flag (e.g. { global: true || false })
@Rich-Harris
Copy link
Member

This seems like a reasonable addition, especially since it doesn't cost anything to people who don't use it. What if we called the new format eval, since that's how it would primarily be used? (Technically expression might be better, but eval is nice and compact, and gets the intention across. Open to bikeshedding!)

In addition, maybe we could add a svelte.create function that simplified this use case:

const MyComponent = svelte.create( input, {...});

// equivalent to
const output = svelte.compile( input, { format: 'eval', ... });
const MyComponent = eval( output.code );

@taylorzane
Copy link
Contributor Author

I like your idea about naming it eval. It definitely makes the most sense.

If we create a create method, I would think having it run eval(svelte.compile(...)) behind the scenes is the preferred route, so that someone could easily do a compile then an eval if they wanted to manipulate the output in any way, and still receive the same output. I'm not sure of a particular use case, but I'm sure they exist.

@Rich-Harris
Copy link
Member

Agree, no reason not to expose it. Only minor distinction is that it should use indirect eval rather than direct eval, since direct eval is a security and minification risk:

(function () {
  var foo = 42;
  console.log(eval('foo')); // 42

  var eval2 = eval;
  console.log(eval2('foo')); // ReferenceError — can't access local vars
}());

@taylorzane
Copy link
Contributor Author

Ah perfect, I can't say I've used eval for anything other than testing purposes, so I appreciate that distinction. I'll get started on this either tonight, or tomorrow.

@taylorzane
Copy link
Contributor Author

I wrote an implementation, and have some test cases. I'd like to do some more testing before I submit that PR. But things are going smoothly! Do you have any preference of indirect eval vs new Function(code)?

@Rich-Harris
Copy link
Member

Great! I think they're basically equivalent — new Function is better if you're going to be running the resulting function many times, but in this case we're only running it once to get the compiled component, so whichever looks neater is best.

@PaulBGD
Copy link
Member

PaulBGD commented Mar 9, 2017

new Function is always better for performance reasons, because it doesn't reuse the scope the eval statement is called in.

@Rich-Harris
Copy link
Member

@PaulBGD interesting, is that also true for indirect eval (as used here)?

@taylorzane
Copy link
Contributor Author

The ECMAScript standard states that an indirect eval uses the global scope, as well as new Function using the global scope.

Is this what you're referring to when you say "doesn't reuse the scope"? If not, could you elaborate on what you meant?

@Rich-Harris
Copy link
Member

It's local scope that's the issue — in the example above (modified to use the (1,eval) trick which I like)...

(function () {
  var foo = 42;
  console.log(eval('foo')); // 42
  console.log((1,eval)('foo')); // ReferenceError — can't access local vars
}());

...anything which was in the global scope would be accessible in both cases. The reason this is important is that direct eval makes it impossible for a minifier to mangle any local variables in case they're referenced inside the eval'd code, and it's a theoretical security risk (if you're eval'ing user input, they could input sendToEvilServer(SECRET_VARIABLE) which wouldn't work with indirect eval or new Function).

@PaulBGD
Copy link
Member

PaulBGD commented Mar 9, 2017

@Rich-Harris
Copy link
Member

If my reading is correct, eval(...) is an optimisation killer but (1,eval)(...) isn't — but we could change it to new Function to be on the safe side?

@taylorzane
Copy link
Contributor Author

I don't see why not. Would just have to be (new Function('return ' + code))().

@taylorzane
Copy link
Contributor Author

I have made the necessary change in #361.

Rich-Harris added a commit that referenced this issue Mar 10, 2017
Changed `create` API method to use `new Function()`
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

3 participants