Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Ensure top-level and string imports only #2

Open
domenic opened this Issue · 22 comments

4 participants

@domenic

The current syntax makes

if (false) { import 'foo'; }
if (Math.random() > 0.5) { import 'foo'; }
import Math.random > 0.5 ? 'foo' : 'false';

work.

I think we want something that only allows the forms

import 'foo';
import AssignmentPattern from 'foo';

and only at top-level of a script (not inside blocks or functions). These of course desugar to require('foo') and let AssignmentPattern = require('foo') (maybe const instead of let).

(Not sure if I understand the difference between AssignmentPattern and BindingPattern but it's one of those two we want.)


Feel free to close if you want this to be a truly minimal translation but I think it'd help your position to have something that browserify can consume without issues like the ones above. In other words I think what you have here is a minimal translation, whereas what I'm proposing is a truly minimal solution.

Looking at it another way, I think the message would be stronger if we joined forces on this and made mmmify better instead of me forking to my own solution.

@VanCoding

Why?

As far as I see it, it does not matter if the import happens inside an if statement or something like this. The file still gets loaded at compiletime, but it doesn't get imported at runtime.

This feature actually makes it possible to import the module right where you need it, so you don't even need to store it in a variable.

Imagine possibilities like this:

(import 'foo').bar();

or this

with(import 'foo'){
    bar();
}

wouldn't it be great?

Update
I don't think your third example is possible, since import only takes static values that must be available at compiletime. Or have I totally misunderstood how this is gona work?

@ForbesLindesay

I agree. The third example definitely needs to be made impossible in that it needs to only accept string liberals. Other than that I'm not too worried, they get loaded but not imported.

If we do want to prevent the first two examples I think we should do so by limiting the depth of the import statement, not by creating another complex bit of syntax to learn. We could just require that import statements do not have conditionals, try/catch, while, switch etc. anywhere in their parent path.

@VanCoding

I think what substack wants to tell us with this proposal is, that he does not want another way of defining variables as import x from y would be. He also does not want a new namespace type. We currently have the possibilites to define variables with var and let, and to do namespace-like things we have objects. Why invent something new?

We can do

var x = import 'x';
let y= import 'y';
var bar = (import 'foo').bar;
myobject.doSomethingAwesome = import 'doSomethingAwesome';

And that's perfectly fine, I think.

@domenic

We currently have the possibilites to define variables with var and let, and to do namespace-like things we have objects. Why invent something new?

Because your code lies otherwise.

This code is lying:

if (false) {
  import 'foo';
}

If this were not magic, but instead normal JavaScript, the stuff inside if (false) would not be executed, so no file would be loaded. It's a bizarre lie that such a statement can not only have an effect on the program, but can cause new files to be loaded into the program---i.e. cause an actual network request, impacting page performance!

If you just prohibit import expressions inside a block, e.g. as @ForbesLindesay suggests, you have created a strange syntax that does not fit with how ECMAScript grammar usually works. There are no other types of expressions which suddenly are disallowed inside different expression contexts. It's a completely non-local restriction, which is hard to reason about.


In my proposal, import AssignmentPattern from 'foo' is sugar for let AssignmentPattern = require('foo') (or maybe const). BUT, by creating a slighly new syntactic form, we gain the ability to never have the program lie, by restricting that static form to the top level. Note that I'm not going crazy, like the ES6 modules proposal! No ImportSpecifierSet microsyntax. Just the usual AssignmentPattern we use for normal variable creation. But it creates a syntax that doesn't let your program lie, and is actually coherent.

Again, there's no new "namespace type." This is just sugar for let or const. Syntax is important and adding new syntax---not just new keywords---is valuable, in order to get benefits like outlined above. It's new semantics that are annoying, e.g. the namespace-like aliasing of the current ES6 proposal, or the bizarre ImportSpecifierSet.

@ForbesLindesay

My suggestion wasn't that you actually execute the code (potentially triggering side effects) I was just suggesting that you download the code. The only side effect that triggers is the additional request to the server (or additional data).

I'd actually rather there be special syntax that was: const AssignmentPattern = import 'foo' and you weren't allowed to use import foo anywhere except on the right hand side of a const declaration. The advantage is readability. People will much more readily grasp what that's doing than import AssignmentPattern from 'foo'

We would also have to require that const AssignmentPattern = import 'foo' was only valid at the top level, but I don't really see a problem with doing so.

@VanCoding

If this were not magic, but instead normal JavaScript, the stuff inside if (false) would not be executed, so no file would be loaded.

Actually, the code would get loaded, yes, but it would never be executed. It would just be loaded for the case it's needed. But since there is a false in the if-statement, it never gets executed. Thus, it also cannot modify the global state.

Is it a lie? I don't think so.

This is just sugar for let or const

Oh, well, I didn't notice this. Sorry!
But i still don't see the benefits of it. It would just be one additional thing that new JavaScript programmers had to learn. Also, you are simply not as flexible as you would be without it. For example, think of the following:

var module = true?import 'a':import 'b';

or

var deps = {
    a:import 'a',
    b:import 'b',
    other:{
        c:import 'c'
    }
};
@ForbesLindesay

@VanCoding That level of flexibility mostly encourages people to write spaghetti code. Although some flexibility is useful. For example I have had to load modules into an object which acts as a mapping from name to module before. With @domenic's proposal that would require a load of temporary variables which would be a shame.

@domenic

Loading code and making network requests via magic lies is no good. Code inside if (false) should do nothing, not make network requests. You lose a little bit of flexibility, I admit. But I think actually being able to tell, statically, what files will be executed is a major benefit, and not loading files you don't want to execute is also a worthwhile goal.

@ForbesLindesay I don't think temporary variables will be necessary, since you can use destructuring with my proposal:

import { foo: bar, baz: [qux, wux] } from "dux";

I also think it's abhorrent to special-case one form of AssignmentExpression and say that form can only occur at top-level, as you propose with const AssignmentPattern = import 'foo' being only valid at top level.

@VanCoding

Code inside if (false) should do nothing, not make network requests.

It doesn't. The requests are made before the code even gets executed.

But if that's still a lie for you then think about the following:

if(false){
    function x(){console.log('hello world');}
}
x();

Is it also a lie? Or is it just the way it works? ;)

@ForbesLindesay

In fairness, I think that example was probably a mistake in the early language design @VanCoding

@domenic I think your suggestion does the reverse of what I want. The example I'd give would be if I wanted to do:

let modules = {
  foo: import 'foo',
  bar: import 'bar'
};

How would that translate to your method?

You have the same restriction of requiring import AssignmentExpression from 'foo' to be at the top level. I get that it has a slight advantage by being different syntax, I just really think there should be an = in there somewhere.

In fact, if we're going to go roughly @domenic's route, I recon:

define AssignmentExpression = import 'foo'

might seem more obvious (where define is substituted for some carefully chosen keyword rather than the first thing that came into my head)

@ForbesLindesay

I think the thing I really don't like isn't the lack of flexibility, but rather that everywhere else where we use the destructuring we have AssignmentExpression = Expression and here we have AssignmentExpression from StringLiteral

@VanCoding

@ForbesLindesay +1 for define, I still think one should not be forced to use it, tough (especially for scenarios like you mentioned above)

@ForbesLindesay

I'd be willing to let that specific scenario go as something that's unusually tricky, or perhaps something that could be solved another way at another time.

Perhaps if we had a nice way to export an import with a name. The equivalent of:

exports.foo = require('./lib/foo');

which I see a lot in code. If we had that and the define syntax (probably not with define as the word though) I'd be happy.

@domenic

@VanCoding:

if(false){
   function x(){console.log('hello world');}
}
x();

This is a syntax error according to the ES5 spec, a runtime error according to the ES6 spec, a syntax error in IE9 and IE10, a runtime error in Firefox, and no error at all in Chrome. (If I recall, haven't tested the browsers recently.) In short, function declarations in blocks are not allowed by ES5; browsers have implemented a variety of incompatible interpretations for them; and in ES6 they are being standardized as like let, i.e. scoped to the block and not to the surrounding function.

Code inside if (false) should do nothing, not make network requests.

It doesn't. The requests are made before the code even gets executed.

That's playing with words. It's like saying var x = 5; doesn't cause a variable to be declared, because hoisting means the variable is already declared by the time the assignment is even executed. It's a distraction from the actual point I'm making. Don't argue like that.

Here's a clearer way of looking at it: before mmmify, and modulo the browser spec-violating nature of functions-in-blocks, every JavaScript program has the same behavior before and after removing all if (false) blocks. You're proposing to change that, causing certain programs with if (false) blocks to cause network or filesystem requests, whereas removing those blocks would remove the filesystem/network requests.

@ForbesLindesay

I'd definitely rather find a way to make if (false) import 'foo' illegal. I just don't want to end up with ugly syntax as a result. I'd be fine with it being a syntax error to put an import statement inside any block, but I do realize that it might confuse beginners, so it's probably not the solution.

@substack
Owner

So much activity! So as I understand the problem, there are 4 different approaches enumerated here, all of which I consider acceptable but I don't think we have enough evidence yet to pick one conclusively.

  1. if (false) var foo = import './foo.js' imports foo

This is how the module currently works and how browserify works too so this approach can actually work in practice quite well. I think the best way of framing what this would be saying that import keywords are always "hoisted" to the top-level for loading purposes but import assignments are not.

  1. if (true) var foo = import './foo.js' is a syntax error

There are some unanswered questions with this variant about what exactly would constitute a block but these could be answered sufficiently with some unit tests. For example in this variant I would want these to all be acceptable:

var bar = (import './foo.js').bar;
var bar = (function (foo) {})(import './foo.js');

but these to be syntax errors:

var bar = [ import './foo.js' ][0];
var bar = true ? import './foo.js' : null;

Pursuing this variant would require an exploration of what exactly would constitute "immediately-executing".

  1. define foo = import './foo'

This is variant would make it more obvious that assignment of module exports is a slightly different kind of beast.
but it has the benefit of resembling assignment with var and let enough to imply the right semantics.

Another good name to consider for define might be declare.

  1. import foo from './foo.js'

Seems to invariably lead to custom destructuring shenanigans that I dislike but it is the most similar to what TC39 already has and has the highest chance of political success. If destructuring were just taken out completely from this syntactical form I could get behind it much more readily.

This variant is the hardest to implement since my patch to esprima only supports custom unary keywords and from makes the syntax into a harder compound expression that will require more upstream patches.


I think the best way to figure out which of these variants is the most workable is to implement all of them and let them stand or fall on their own merits. The index.js in this repo is just 28 lines so it should be pretty easy to take that as a starting point.

@domenic

@substack Awesome to see we're basically on the same page.

Seems to invariably lead to custom destructuring shenanigans that I dislike

I don't think "invariably". Indeed my whole point in proposing this was that, while TC39 wants their ImportSpecifierSet pseudo-destructuring microsyntax, I want just plain old destructuring with no specialness at all, i.e. reuse the AssignmentPattern grammar element. Given this, is it still less attractive to you than the others?

@domenic

One thing we can pretty much agree on, from what I see, is that it should always be a string, not an expression (i.e. import true ? 'foo.js' : null is no good). So we can at least fix that as a starting point.

@substack
Owner

So long as I can usually just ignore the import destructuring and rely on the simpler case for single-export this isn't too important of a point. I completely agree that import 'path' should be the only supported argument form.

@ForbesLindesay

@substack's summary seems accurate. We do seem to all agree on pure literal strings not expressions. We also all seem to agree on single export. Both good :smile:

I also agre with @domenic That we should do something which re-uses the AssignmentPattern for destructuring. I don't think this should cause too many problems for static analysers like browserify as they can just ignore what's being assigned to once it's been parsed. All they care about is what's being imported. You also don't need to worry about it if you're not using it.

@ForbesLindesay

I also agree that we should somehow prevent import appearing inside if statements, case statements, any of the looping constructs, and functions. The remaining question is therefore how to prevent that while incurring the minimum overhead on people learning or who are just using them normally at the top of the file.

@ForbesLindesay

Another possible option:

declare {
  foo = import './foo.js';
  bar = import './bar.js';
}

You could restrict to one declare block per module, and require it to be in the top scope. Assuming most modules end up needing to import several different things (I hardly ever import fewer than 3), it's actually pretty succinct too. The only problem is I don't think declare or define etc. are reserved words in ES5.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.