Skip to content
This repository was archived by the owner on Feb 22, 2018. It is now read-only.

Atom plugin broken #396

Closed
jmesserly opened this issue Dec 11, 2015 · 9 comments
Closed

Atom plugin broken #396

jmesserly opened this issue Dec 11, 2015 · 9 comments

Comments

@jmesserly
Copy link
Contributor

It looks like the destructuring commit broke Atom (c9d909c) ... probably because they're on an older version of Chrome.

I think the new code has fallback logic already in some cases, so we could likely add a flag to turn that on always.

CC @devoncarew @vsmenon

@jmesserly jmesserly added the js label Dec 11, 2015
@ochafik ochafik self-assigned this Dec 11, 2015
ochafik pushed a commit that referenced this issue Dec 11, 2015
…rams (issue #396)

Right now one also needs to regenerate the runtime:
./tools/build_sdk --no-destructure-named-params
ochafik pushed a commit that referenced this issue Dec 11, 2015
…rams (issue #396)

Right now one also needs to regenerate the runtime (see #397):
./tools/build_sdk --no-destructure-named-params
@ochafik
Copy link
Contributor

ochafik commented Dec 11, 2015

Sent a PR with a flag that disables destructuring (and filed #397 to track multiple runtimes).

@devoncarew, if running build_sdk.sh as part of the plugin's build is not an option, then I can flip _DESTRUCTURE_NAMED_PARAMS_DEFAULT to false and we'll re-enable it later.

@devoncarew
Copy link
Contributor

if running build_sdk.sh as part of the plugin's build is not an option

It would be a lot more convenient to not have to do rebuild it :)

@vsmenon
Copy link
Contributor

vsmenon commented Dec 11, 2015

@devoncarew It looks like destructuring is behind a separate "--harmony-destructuring" in Chrome 45. Is that possible to turn on?

If not (or it doesn't work), we can disable it by default until Atom rolls forward.

@ochafik does anything break without destructuring?

vsmenon added a commit that referenced this issue Dec 11, 2015
Allow disabling named param destructuring with --destructure-named-params (issue #396)
@ochafik
Copy link
Contributor

ochafik commented Dec 11, 2015

Destructuring fixes a pattern that breaks Closure's advanced mode, but it's not ready yet anyway, so can definitely compile the sdk without it (it will just grow 1% bigger ;-)).

If that's an acceptable middle ground, I suggest building the sdk without it, but leaving it as a default (and have the Atom build to add the --no-destructure-named-params flag), this way the generated code looks nicer by default.

@devoncarew
Copy link
Contributor

sgtm!

I tried adding this to my start-up script:

var v8 = require('v8');
v8.setFlagsFromString('--harmony-destructuring');

which changed the failure mode, but did hit another exception.

ochafik pushed a commit that referenced this issue Dec 11, 2015
This is so the Atom plugin doesn't need to regenerate the SDK (it still
needs to be compiled with --no-destructure-named-params, though, until
Atom is updated to more ES6-compliant node/Chrome).
vsmenon added a commit that referenced this issue Dec 11, 2015
Compile the sdk with --no-destructure-named-params (#396)
@vsmenon
Copy link
Contributor

vsmenon commented Dec 14, 2015

@devoncarew Let us know if this is still an issue. Destructuring has been disabled by default.

We're also now regressing on Electron (the Chrome 47 version - non-trivial to move that earlier as our karma/travis version doesn't make it easy to run older Electron with --harmony). This would have caught the destructuring issue early.

@devoncarew
Copy link
Contributor

Thanks! Will re-test w/ the latest DDC.

@jmesserly
Copy link
Contributor Author

tentatively closing, let us know if it reoccurs!

@devoncarew
Copy link
Contributor

Sorry, yup, it's working for me again.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

No branches or pull requests

4 participants