nodegui / qode

DEPRECATED: Please see https://github.com/nodegui/qodejs instead
https://nodegui.github.io/nodegui/#/tutorial/application-architecture?id=qode
91 stars 10 forks source link

dynamic imports never resolve, iff you import anything from @nodegui/nodegui in the script doing the importing #34

Open valyrie97 opened 3 years ago

valyrie97 commented 3 years ago

Version information:

node: 16.0.0
qode: 14.17.0
yarn: 1.22.4
npm:  7.10.0
Screen Shot 2021-07-21 at 21 54 13

reproduction steps:

And you will get no output. However, if you remove line 1 from test.js, suddenly everything works as intended. Swapping the order also is seemingly irrelevant, the error persists whether i try to do the dynamic load first or not. Even when attempting to use top level await to force the dynamic import to happen before the static import.

valyrie97 commented 3 years ago

update: this seems to be a more general problem with Promises. if you setTimeout, everything works normal. however, creating a loop such as

for(let i = 0; i < 5; i ++) {
    console.log('waiting 1 second');
    await new Promise(res => setTimeout(res, 1000));
}

causes the execution to only return to the resolution of the promise once ive moved to another virtual desktop. (3/4 finger scroll over to a fullscreen app, for example)

This leads me to believe that execution of promise then callbacks is the culprit here.

valyrie97 commented 3 years ago

Update: After a lot of tinkering, I've isolated that the execution of promise.then(cb) callbacks do eventually get called, but only when the window is updated. This is why when moving between screens, and its focus state changed, promises would resolve.

my current workaround to this (which is terrible for performance), is to have an infinite loop that calls win.update()

function updateLoop() {
    win.update();
    win.repaint();
    setTimeout(updateLoop, 100);
}
updateLoop();

Edit: turns out one needs both a repaint, and an update call to fully allow Promises to resolve. updated workaround code.

a7ul commented 3 years ago

Yep this looks like a bug. Can you see if an older version of qode works for you? This looks like it might be an issue with recent node version upgrade.

valyrie97 commented 3 years ago

as i must import nodegui to get the bug to happen, and im getting unrelated errors trying to use a version of qode mismatched with the nodegui version, Im testing each nodegui version here. Each test creates 5 promises resolved after a setTimeout of 0ms.

Each install i rm -rfed the node modules directory, to ensure cleanliness, and always used the version of qode store in node_modules/.bin.

The setup seems to be extremely finicky, so I've also included my test file.

app.js

// swap out import / require for app.js / app.mjs testing
import '@nodegui/nodegui';
// require('@nodegui/nodegui');

function unoMomento() {
  return new Promise(res => {
    setTimeout(res, 0)
  })
}

function doPromise() {
  unoMomento().then(_ => {
    console.log('resolved')
  })
}

doPromise();
doPromise();
doPromise();
doPromise();
doPromise();

setTimeout(_ => {
  console.log('done?')
}, 1000)
nodegui version Resolved Promises ESM CJS
0.34.0 0 0
0.33.3 0
0.33.0 0
0.32.0 0
0.31.0 0
0.30.3 0
0.30.0 *
0.29.0 *
0.28.1 0
0.27.0 0
0.26.0 0
0.25.0 0 0
0.24.0 0 0
0.23.1 0 0
0.23.0 0 0
0.22.0 0 0
0.21.0 0 0
0.15.5 0 0
0.15.4 0 0
0.15.3 0 0
0.15.2 ** **
0.15.1 ** **
0.15.0-alpha-4 ** **
0.15.0-alpha ** **
0.13.4 - 5
0.12.1 - 5
0.6.5 - 5

* came back with an error about not being able to find the binding. '../../../build/Release/nodegui_core.node'

** came back with an error about my QT setup

Blanks are untested; Dashes are ERR_REQUIRE_ESM (assuming an old enough version of node that import syntax wasn't yet mature)

Hope this info is helpful!

a7ul commented 2 years ago

This is issue is solved in master branch of qode. But since I am having some issues with windows build I haven't yet made a release. In the meantime can you look if this binary solves the issue for you

https://github.com/nodegui/nodegui/issues/862#issuecomment-892475695