qode icon indicating copy to clipboard operation
qode copied to clipboard

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

Open valyrie97 opened this issue 2 years ago • 5 comments

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:

  • set your package.json to have "type": "module"

test.js

import { QMainWindow } from '@nodegui/nodegui';
import('./imported.js').then(console.log).catch(console.log);

imported.js

export const test = 5;

qode test.js

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 avatar Jul 22 '21 02:07 valyrie97

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 avatar Jul 22 '21 05:07 valyrie97

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.

valyrie97 avatar Jul 23 '21 00:07 valyrie97

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.

a7ul avatar Jul 23 '21 07:07 a7ul

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!

valyrie97 avatar Jul 23 '21 17:07 valyrie97

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

a7ul avatar Aug 04 '21 08:08 a7ul