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

Out-of-order statements… are out-of-order. #21

Closed
mbostock opened this issue Jun 6, 2015 · 3 comments
Closed

Out-of-order statements… are out-of-order. #21

mbostock opened this issue Jun 6, 2015 · 3 comments

Comments

@mbostock
Copy link
Contributor

mbostock commented Jun 6, 2015

I’m seeing a problem in d3-color where statements are output out-of-order, and so the code crashes. For example:

__prototype.rgb = ;

 // code continues

var __prototype = Hsl.prototype = new Color;

Notice that __prototype is defined after the assignment, and so the code throws a TypeError: Cannot set property 'rgb' of undefined.

The recent statements.sort in c0a7640 for 0.7.3 may be the cause. This looked suspicious:

// within a module, statements should preserve their original order
// TODO is this foolproof?
this.statements.sort( ( a, b ) => {
  if ( a.module !== b.module ) return -1; // no change
  return a.index - b.index;
});

Forgive my lack of familiarity with the code, but I am unsure what you are trying to achieve by returning -1, here. That would mean to always put a before b, and this logic is nondeterministic and potentially contradictory since you don’t control the underlying sort algorithm and which elements are being compared.

If you don’t care about the relative order of two statements in different modules, you might instead return zero or NaN (and perhaps rely on a stable sorting algorithm). Or, perhaps just define an arbitrary order of modules, such as by id, and compare those before comparing by index.

Anyway, that’s just my wild guess from looking at the commits since 0.7.2. I apologize if this is a distraction.

@Rich-Harris
Copy link
Contributor

Whoops. My understanding of [].sort was way off the mark, thanks for the explanation. The intent was just to make sure that...

var a = 1;
export default a;
a = 2;

...would maintain the same order, since (due to the fact that export default a isn't a live binding like export {a}) it effectively needs to be rewritten as this:

var a = 1;
var _a = a;
a = 2;

It's a bit of a nuisance edge case (and rare, as far as I can see) but specs are specs. Ordinarily statements preserve the correct order naturally, but export default trips things up.

I apologize if this is a distraction.

Far from it - being able to catch these things at this stage of the library's development is absolutely invaluable. I apologize for breaking your builds! Am working on a solution for this issue.

@Rich-Harris
Copy link
Contributor

Okay, I've come up with what seems like a more appropriate solution - we have some special logic for dealing with cases like the above. Rather than attempting to reorder statements at a bundle level, the expansion of export default a is modified in place (if necessary) such that the export declaration appears before any later statements from the same module. The d3-color tests now pass, so I've released this as 0.7.4 - feel free to reopen this issue if it turns out the bug isn't quite squashed.

@mbostock
Copy link
Contributor Author

mbostock commented Jun 6, 2015

Thanks. Looks good!

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