allow whitelisted globals

[Rich-Harris]

Fixes #185. One side-effect – Svelte would always use the global, rather than a top-level data property with the same name. I think that's probably okay – including properties or helpers with names like Array and NaN would be wildly confusing and should be discouraged at any rate – but I'd been keen to hear if other people agree.

[codecov-io]

Current coverage is 92.05% (diff: 100%)

Merging #186 into master will decrease coverage by 0.39%

@@             master       #186   diff @@
==========================================
  Files            61         59     -2   
  Lines          1697       1662    -35   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
- Hits           1569       1530    -39   
- Misses          128        132     +4   
  Partials          0          0          

Powered by Codecov. Last update 14ef2ed...1cfbfb0

[Conduitry]

Would we want this list to be configurable at compile time? Are you worried that would encourage poor practices?

[Swatinem]

I actually think this is something that could be resolved at runtime with a helper function that selects the property from either bindings defined inside the template (loop value/key), the passed in props, (some possible context if we come up with a way how to best add this feature), or window.

[Rich-Harris]

@Conduitry what kind of poor practices do you mean? I think it's okay to use things like Math and encodeURIComponent without any boilerplate, since you'd do the same in regular JS.

@Swatinem yeah, did wonder about that. Loop context and index are already accounted for statically, as are helpers. So it'd be something like this:

if ( contexts[ name ] ) {
  dependencies.push( ...contextDependencies[ name ] );
  if ( !~usedContexts.indexOf( name ) ) usedContexts.push( name );
} else if ( indexes[ name ] ) {
  const context = indexes[ name ];
  if ( !~usedContexts.indexOf( context ) ) usedContexts.push( context );
} else if ( globalWhitelist[ name ] ) {
  dependencies.push( name );
  generator.code.prependRight( node.start, `( '${name}' in root ? root.` );
  generator.code.appendLeft( node.end, ` : ${name} )` );
  if ( !~usedContexts.indexOf( 'root' ) ) usedContexts.push( 'root' );
} else {
  dependencies.push( name );
  generator.code.prependRight( node.start, `root.` );
  if ( !~usedContexts.indexOf( 'root' ) ) usedContexts.push( 'root' );
}

– i.e. {{Math.min(1,2,3)}} would become ( 'Math' in root ? root.Math : Math ).min(1,2,3)`. Not ideal, if you have lots of them, but maybe not terrible either.

[Conduitry]

@Rich-Harris I meant that if the user can choose which globals to whitelist for direct access, they could use this to access any global variable they wanted, which seems to be a bad way to pass data in and out of components. There's nothing wrong with the globals you are whitelisting in this PR - the bad practice concern was if users can add their own. I'm not sure what a good balance is between "helping users help themselves if they want to have easy access to other browser environment APIs" and "helping users not make bad architecture decisions that would make the component more confusing to use".

[Rich-Harris]

@Conduitry ah, gotcha. No need for whitelist configuration in that case – people are free to add whatever globals they want to data or helpers:

<p>{{xssVulnerability(userInput)}}</p>

<script>
  export default {
    helpers: {
      xssVulnerability
    }
  };
</script>

[Swatinem]

Having to add them explicitly seems like a good idea as well.

The ideas I have are a bit more involved, and currently I can’t think of a better idea to those than to either reimplement the prototype chain, or have a bunch of Object.assigns all around. Not a really good idea for perf I would argue.

I will elaborate on a new issue.

[Rich-Harris]

I've updated the PR so that local data always overrides globals, but globals can still be accessed without any boilerplate