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

Loose mode for import * as #144

Closed
lukeapage opened this issue Sep 25, 2015 · 10 comments
Closed

Loose mode for import * as #144

lukeapage opened this issue Sep 25, 2015 · 10 comments

Comments

@lukeapage
Copy link
Contributor

Hi,

When using import * as we get the following output

var a = {
    get foo() { return foo; }
}

which is "correct", however a shorter form would be

var a = { foo: foo };

Would you consider a option that didn't bother with writing getters.. since I very much doubt any code will depend on not being able to set things on that object?

@Victorystick
Copy link
Contributor

@Rich-Harris has considered avoiding getters, at least for exports and ES3 environments. I don't think that it'd be possible in general, as it breaks with the ECMAScript spec for exports that get reassigned.

However, it would be possible to optimise const exports. For instance,

export const foo = {};

when imported as import * as a could generate

var a = { foo: foo };
Object.defineProperty(a, 'foo', {
  enumerable: true,
  value: foo
});

since we know that foo isn't going to be reassigned. It isn't shorter exactly, but wouldn't include the overhead of a getter function.

@lukeapage
Copy link
Contributor Author

I don't think that it'd be possible in general, as it breaks with the ECMAScript spec for exports that get reassigned.

Could you expand on that? I understand it would no longer follow the spec, thats why I am suggesting an option along the lines of babel's loose mode (and its strict mode = false) which make the code shorter/faster but don't follow the spec perfectly.

It would be more work for rollup, but I would imagine the code that generates this is in one or two specific places. I'd be interested in any downsides to that for the consumer or anything you think might break.

@Victorystick
Copy link
Contributor

With the loose more you're suggesting, Rollup could generate a bundle like the one below. The semantics state that the access of the foo variable should be the same wether it's through the namespace a, directly through the imported foo. But this is broken by the loose mode.

//--- foo.js
export function nextFoo () { foo++; }
export var foo = 1;

//--- main.js
import * as a from './foo.js';
import { foo, nextFoo } from './foo.js';

nextFoo();

assert.equal( a.foo, foo );

//--- proposed.js
function nextFoo () { foo++; }
var foo = 1;

var a = { foo: foo };

nextFoo();

assert.equal( a.foo, foo );

Current Rollup will generate the below bundle, and completely optimise the namespace away.

//--- current.js
function nextFoo () { foo++; }
var foo = 1;

nextFoo();

assert.equal( foo, foo );

@lukeapage
Copy link
Contributor Author

Is that because you optimize in stages and a later stage wouldn't be able to tell that a was not required?

Ideally from your example, rollup would generate the same thing as now, but I wouldn't end up with hundreds of

var a = {
    get foo() { return foo; }
}

in my output from every import * as x. At the moment to get what I want I have to write this..

import * as foo from 'foo';
export default {
    bar: foo
};

or

export * as bar from 'foo';

(proposal from es2016)

as

import {foo_a, foo_b} from 'foo';
export default {
    bar: {
        a: foo_a,
        b: foo_b
    }
};

@Victorystick
Copy link
Contributor

I see. So, what you really want is to be able to use the ES2016 export extensions? We're tracking it in #82. Rollup could easily handle your example, and I'd be more than happy to add support for it, but unfortunately Acorn doesn't have a plugin (I know of) that can parse the export extensions.

If one existed, it'd be another issue entirely. 😄

@Rich-Harris
Copy link
Contributor

However, it would be possible to optimise const exports

Good thinking. In fact, we could determine whether an export is in fact possible to change, rather than relying on const (though const could save us the effort) – there's already some code in there that does the same check to determine whether a default export needs to be aliased or not, which could probably be reused. If a namespace-exporting module had no exports that could change, we could do away with the getters (though there might be some spec details around enumerability/configurability that we need to consider).

It would be good to have a loose mode anyway, for people who still have to support IE8 as well as for people who really don't want to mess around with accessors even if the exports could change. In Esperanto the option was called _evilES3SafeReExports (esperantojs/esperanto#89).

@lukeapage
Copy link
Contributor Author

I see. So, what you really want is to be able to use the ES2016 export extensions? We're tracking it in #82. Rollup could easily handle your example, and I'd be more than happy to add support for it, but unfortunately Acorn doesn't have a plugin (I know of) that can parse the export extensions.

no, I was just adding the 2016 export extension example to make my usecase clearer.. sorry this probably confused things.

I show 2 equivalent pieces of code for es2015 and es2016 and then what I have to write in es2015 to get rollup to output what I would like it to..

It would be good to have a loose mode anyway, for people who still have to support IE8 as well as for people who really don't want to mess around with accessors even if the exports could change. In Esperanto the option was called _evilES3SafeReExports (esperantojs/esperanto#89).

+1 and I don't mind a name like that, though I'd rather it was es3 specific since I want it to get nicer output

@Rich-Harris
Copy link
Contributor

As of 0.18, namespace blocks will only use getters for exports that could be reassigned or updated. Unfortunately less.js still doesn't build correctly (for unrelated reasons – investigating) but hopefully that does a good enough job of resolving this issue

@lukeapage
Copy link
Contributor Author

As of 0.18, namespace blocks will only use getters for exports that could be reassigned or updated.

Perfect, have tried it out and this looks good to me.

Unfortunately less.js still doesn't build correctly (for unrelated reasons – investigating)

It builds and tests okay to me. It may be just because I haven't finished switching everything to rollup, so npm test doesn't work on that branch.

You may be interested in filesize specs (not so important for less but I was interested regardless):

                           rollup     browserify
as-is                      354kb      341kb
minified (uglify)          118kb      131kb
minified and gzipped       40kb       42Kb

@Rich-Harris
Copy link
Contributor

It builds and tests okay to me

Yep, I fixed the other issue (9a8f331). Thanks for sharing the filesizes – good to know it ends up smaller with Rolllup

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants