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

ReferenceError: g is not defined #127

Closed
HighTide opened this issue Mar 24, 2021 · 19 comments
Closed

ReferenceError: g is not defined #127

HighTide opened this issue Mar 24, 2021 · 19 comments
Milestone

Comments

@HighTide
Copy link

HighTide commented Mar 24, 2021

I get the following error:

ReferenceError: g is not defined
    at Emd (elk-worker.min.js:5037)
    at Iqd (elk-worker.min.js:5685)
    at h.dispatch (elk-worker.min.js:5984)
    at h.saveDispatch (elk-worker.min.js:5984)
    at elk-worker.min.js:5985
    at ZoneDelegate.invokeTask (zone-evergreen.js:399)
    at Object.onInvokeTask (core.js:41264)
    at ZoneDelegate.invokeTask (zone-evergreen.js:398)
    at Zone.runTask (zone-evergreen.js:167)
    at invokeTask (zone-evergreen.js:480)
    at ZoneTask.invoke (zone-evergreen.js:469)
    at timer (zone-evergreen.js:2552)
    at resolvePromise (zone-evergreen.js:798)
    at resolvePromise (zone-evergreen.js:750)
    at zone-evergreen.js:860
    at ZoneDelegate.invokeTask (zone-evergreen.js:399)
    at Object.onInvokeTask (core.js:41264)
    at ZoneDelegate.invokeTask (zone-evergreen.js:398)
    at Zone.runTask (zone-evergreen.js:167)
    at drainMicroTaskQueue (zone-evergreen.js:569)
    at invokeTask (zone-evergreen.js:484)
    at ZoneTask.invoke (zone-evergreen.js:469)
    at timer (zone-evergreen.js:2552)

This issue is caused by the elk-worker.min.js file. the elk-worker.min.js:formatted has the following code:

function Emd(a, b, c) {
    var d, e, f, h, i, j;
    d = smd(a, (e = (ddd(),
    f = new Bkd,
    f),
    !!c && zkd(e, c),
    e), b);
    kgd(d, Ald(b, Xoe));
    Hmd(b, d);
    Cmd(b, d);
    Imd(b, d);
    g = null;
    h = b;
    i = xld(h, 'ports');
    j = new ind(a,d);
    emd(j.a, j.b, i);
    Dmd(a, b, d);
    ymd(a, b, d);
    return d
}

It uses the var g, however, g is not being defined as var. It can be fixed manually by adding g to the var declarations, like this.

function Emd(a, b, c) {
    var d, e, f, g, h, i, j;
    d = smd(a, (e = (ddd(),
    f = new Bkd,
    f),
    !!c && zkd(e, c),
    e), b);
    kgd(d, Ald(b, Xoe));
    Hmd(b, d);
    Cmd(b, d);
    Imd(b, d);
    g = null;
    h = b;
    i = xld(h, 'ports');
    j = new ind(a,d);
    emd(j.a, j.b, i);
    Dmd(a, b, d);
    ymd(a, b, d);
    return d
}

I would normally create a pull request, however as far as I can tell the elk-worker gets generated from the GWT code and I have no idea how this issue can be solved.

@uruuru
Copy link
Member

uruuru commented Mar 24, 2021

Could this be related to #96?

You are correct in that the code is fully generated.

@HighTide
Copy link
Author

Yes, it looks like it's the same problem. I'm also using it inside an angular project so strict mode is on.

@uruuru
Copy link
Member

uruuru commented Apr 7, 2021

As explained in #96, I'm not aware of a "fix" other than excluding elkjs from being handled in strict mode. I'm open for suggestions though.

@uruuru uruuru closed this as completed Apr 7, 2021
@uruuru uruuru added the wontfix label Apr 7, 2021
@amcdnl
Copy link
Contributor

amcdnl commented Jun 7, 2021

Removing strict mode will resolve the issue - depending on what build system you use it might add it automatically. For example, rollup does but u can turn this off like:

module.exports = [
  {
    input: './src/Embed/Embed.tsx',
    output: [
      {
        file: pkg.browser,
        sourcemap: true,
        format: 'umd',
        name: 'flow',
        strict: false
      }
    ],

@Qix-
Copy link

Qix- commented Jun 13, 2021

Browser are strict by default these days, and there's no way to run in non-strict mode. Why that affects execution times is very strange, though.

At any rate, there's a missing variable in the declared var a, b, c, d, etc clause above in this function (and a few other functions). Adding those variables manually fixes the issue and 100% will not affect runtime. Thus, this seems to be a bug in whatever generates the javascript. Clearly they're pre-declaring variables (which isn't required in non-strict mode) but are missing several (which causes bugs within strict mode).

This is not the only instance - we had to fix about four or five different instances of this in order to get it to run.

This is preventing us from using Elk without vendoring it in and manually adjusting things. I'll have to fork and see if I can't get it building myself :/

@Qix-
Copy link

Qix- commented Jun 13, 2021

I wrote a patcher for the build process - the repository is at my https://github.com/qix-/elkjs fork. This fixes the broken GWT output (maybe not completely - you'll need to add to worker-patch.js in the root if it doesn't).

I also removed all of the extra layout algorithms as an additional, singular commit. This cut down the output size by almost a megabyte. If you need anything other than layered, you can git revert bc9f97e19e91f6cde67f2ad988bfa8884aa24ffd and re-build.

If you intend to publish on your own, revert the HEAD commit as well and make whatever changes you need to the package.json yourself.

Hope that helps anyone that comes across this bug that nobody (neither the ElkJS nor GWT team) wants to fix.

@uruuru
Copy link
Member

uruuru commented Jun 16, 2021

Thus, this seems to be a bug in whatever generates the javascript.

I agree. If somebody finds the root cause, is able to address it within GWT and triggers a new release of GWT, I'd the first to create a new elkjs release.

Injecting the missing variables as a patch after the actual build seems to be a reasonable workaround to me. Would you be open to create a PR with just this change?

Regarding removing the other layouters: they may be "superfluous" to you but not in general. #6 also mentions the size differences. If somebody has a suggestion for a low-effort or automated process to publish variants (for instance, layered and all as a start), I'm happy to integrate it.

@Qix-
Copy link

Qix- commented Jun 16, 2021

Regarding removing the other layouters: they may be "superfluous" to you but not in general.

Yes of course, I didn't mean to suggest they were superfluous in general, just to my use-case - hence why I gave instructions on how to revert that change if someone wanted to use my version.

I will PR in a bit.

@uruuru uruuru removed the wontfix label Jun 20, 2021
@uruuru uruuru added this to the 0.7.2 milestone Jun 20, 2021
@uruuru uruuru reopened this Jun 20, 2021
@uruuru
Copy link
Member

uruuru commented Jun 20, 2021

I just realized that a better place to add the missing variables is likely below here

out.print(" FAKE ELEMENTS GWT ASSUMES EXIST");

@uruuru
Copy link
Member

uruuru commented Oct 31, 2021

Should be fixed in 0.7.3-dev.

@uruuru uruuru closed this as completed Oct 31, 2021
@KonradLinkowski
Copy link

@uruuru do you plan to release 0.7.3 as a full version?

@uruuru
Copy link
Member

uruuru commented Feb 26, 2022

There are plans for a new elk release within the next month. A subsequent elkjs release would include the fixes.

@luanlucho
Copy link

Any solutions yet?

@soerendomroes
Copy link
Member

soerendomroes commented Jun 13, 2022

@luanlucho elkjs 0.8.1 is released (but not yet tagged as latest), have you tried whether this fixes the issue?

@livwvil
Copy link

livwvil commented Jun 16, 2022

elkjs 0.8.1 fixes this issue for me

@jyc
Copy link

jyc commented Jun 20, 2022

Thanks for fixing this uruuru! If you enable donations or sponsorships on GitHub people might be interested!

@tunesmith
Copy link

Just pointing out that npm install elkjs still installs 0.7.1, but changing the version number in package.json does find 0.8.1 successfully.

@waynelwz
Copy link

I have building by vite, elkjs 0.8.1 is not work for me, I resolved it tricky by

EDIT:
just write this on App.js or App.tsx:
window.g = null;
window.i = null;
elkjs: 0.7.1

@Qix-
Copy link

Qix- commented Aug 29, 2022

The dist-tag is next, by the way. latest is still tagged under the 0.7 version.

Maintainers, mind pushing this straight to latest? It'll cut down on the confused responses here.

Everyone else, use npm install elkjs@next for now, if you want.

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