Don't create custom elements in main and fix various small issues on tests by FabioRosado · Pull Request #747 · pyscript/pyscript

FabioRosado · 2022-09-04T00:54:03Z

This PR fixes a few things:

the 'in undefined' error that we have seen when running tests
the flakiness of the reply tests
the test_panel_stream tests (cannot read properties of undefined)
wait a short time to allow the loaded script to be inserted into head

Hopefully, you don't mind this wall of text, explaining the reasoning behind the decisions in the PR 😄

runAfterRuntimeInitialized running immediately on page load

After adding a bunch of console.traces I've noticed that runAfterRuntimeInitialized was being called from main.js which was a bit odd, following the code path, it seems that it was being run when we were creating the py-button custom element in const xPyButton = customElements.define('py-button', PyButton); See the console output for a better visual.

Console output with traces

adding initializer 
async function mountElements()
stores.ts:22:12
added initializer 
async function mountElements()
stores.ts:24:12
adding post initializer 
async function initHandlers()
stores.ts:27:12
added post initializer 
async function initHandlers()
stores.ts:29:12
RUNTIME READY pyenv.ts:8:12
initializers set runtime.ts:9:12
post initializers set runtime.ts:14:12
scripts queue set 2 runtime.ts:19:12
connected pyscript.ts:56:16
console.trace() inside runAfterRuntime base.ts:170:16
    runAfterRuntimeInitialized base.ts:170
    connectedCallback pybutton.ts:57
    <anonymous> main.ts:18
    <anonymous> pyscript.js:27234
console.trace() pyconfig constructor pyconfig.ts:22:16
    PyConfig pyconfig.ts:22
    createElement hello_world.html:1
    <anonymous> main.ts:28
    <anonymous> pyscript.js:27234
console.trace() pyconfig connected callback pyconfig.ts:27:16
    connectedCallback pyconfig.ts:27
    <anonymous> main.ts:29
    <anonymous> pyscript.js:27234
Uncaught TypeError: right-hand side of 'in' should be an object, got undefined
    runAfterRuntimeInitialized base.ts:172
    subscribe index.mjs:50
    runAfterRuntimeInitialized base.ts:171
    connectedCallback pybutton.ts:57
    <anonymous> main.ts:18
    <anonymous> pyscript.js:27234
base.ts:172:16
config set! runtime.ts:27:16
config set 
Object { autoclose_loader: true, runtimes: (1) […] }
pyconfig.ts:41:16
Initializing runtimes... pyconfig.ts:53:16
console.trace() pyodide.ts:40:16
    loadInterpreter pyodide.ts:40
    initialize runtime.ts:55
    loadRuntimes pyconfig.ts:59
    (Async: EventListener.handleEvent)
    loadRuntimes pyconfig.ts:58
    connectedCallback pyconfig.ts:42
    <anonymous> main.ts:29
    <anonymous> pyscript.js:27234
creating pyodide runtime pyodide.ts:41:16
Python initialization complete pyodide.asm.js:10:271183
loading micropip pyodide.ts:51:16
Loading micropip, pyparsing, packaging, distutils pyodide.asm.js:10:136383
Loaded micropip, packaging, pyparsing, distutils pyodide.asm.js:10:137532
loading pyscript... pyodide.ts:53:16
done setting up environment pyodide.ts:58:16
RUNTIME READY pyenv.ts:8:12
Collecting nodes to be mounted into python namespace... pyscript.ts:219:12
evaluate base.ts:100:16
console.trace() 
Object { src: "https://cdn.jsdelivr.net/pyodide/v0.21.2/full/pyodide.js", name: "pyodide-default", lang: "python", interpreter: {…}, globals: Proxy }
base.ts:106:20
    evaluate base.ts:106
    initialize runtime.ts:77
    loadRuntimes pyconfig.ts:59
    (Async: EventListener.handleEvent)
    loadRuntimes pyconfig.ts:58
    connectedCallback pyconfig.ts:42
    <anonymous> main.ts:29
    <anonymous> pyscript.js:27234
----> changed out to py-493bc6ca-5a1b-d659-6e60-a158dafd047c true 2 pyodide.asm.js:10:198133
Element.write: 09/03/2022, 22:08:34 --> True pyodide.asm.js:10:198133
----> reverted 3 pyodide.asm.js:10:198133
scripts queue set runtime.ts:19:12
------ loader closed ------ runtime.ts:84:20
Collecting nodes... pyscript.ts:183:12
===PyScript page fully initialized=== runtime.ts:89:16
registered handlers pybutton.ts:60:20
Source map error: Error: request failed with status 404
Resource URL: https://cdn.jsdelivr.net/pyodide/v0.21.2/full/pyodide.js
Source Map URL: pyodide.js.map

Initially I wrapped the customElements in a setTimeout and waited for 2.5s. This was sufficient most of the time, but there were some odd times that it wasn't. Increasing it to 3s made the button appear after the page loaded, which looked odd.

The main.js file will only create elements for PyScript, PyConfig, PyLoader and PyEnv. and the rest of the elements will be created once the runtime loads successfully and we closed the loader. With this change, the elements were always rendered successfully on the page and the error Cannot use 'in' operator to search for 'run' in undefined stopped.

Initially, the createCustomElements was in the utils.ts but that caused jest to fail with a strange behaviour, so this function was moved to its own file.

Removing the time.sleep from tests

While working on the tests last week (or two?) I added a few time.sleep because events weren't being triggered correctly. After looking into the py-button integration test and noticing that the clicks weren't registered, I started looking into the PyScriptTest and the PyScript code.

The PyScriptTest will wait for ===PyScript page fully initialized=== to be printed in the console, once playwright sees the log, it starts the tests and in this case, the clicking. But the script hasn't been added to the head of the page yet, so no event is registered. Here's the relevant code:

pyscript/pyscriptjs/src/components/pyconfig.ts

Line 68 in e31e03a

document.head.appendChild(script);

Adding a small page.wait_for_timeout was enough for the tests to start passing 😄 I'm wondering if perhaps we can make this timeout even smaller?

In a few tests, I've also added a self.page.wait_for_selector which seems to work nicely so playwright will wait until the element is loaded before trying to click or check the contents of the selector.

Please let me know if you would like me to change anything here (or if my assumptions/observations are incorrect 😄 )

Fixes: #673, #677, #726

antocuni

Wow, very good debugging/investigation in this PR, thank you!
I am a bit confused about this though:

The PyScriptTest will wait for ===PyScript page fully initialized=== to be printed in the console, once playwright sees the log, it starts the tests and in this case, the clicking. But the script hasn't been added to the head of the page yet, so no event is registered. Here's the relevant code:

I don't understand how this is possible.
(In the following I will put links to the source code in #737 because the logging is better and easier to follow, but the core logic is unchanged).
This is the workflow:

PyConfig.connectedCallback() is called, and it calls this.loadRuntimes()

pyscript/pyscriptjs/src/components/pyconfig.ts

Lines 46 to 50 in 4694602

    
               appConfig.set(this.values); 
        
               logger.info('config set:', this.values); 
        
               this.loadRuntimes(); 
        
           }

in loadRuntimes, we create the <script> tag to download pyodide, add a load event listener and append it to document.head:

pyscript/pyscriptjs/src/components/pyconfig.ts

Lines 62 to 73 in 4694602

    
           loadRuntimes() { 
        
               logger.info('Initializing runtimes'); 
        
               for (const runtime of this.values.runtimes) { 
        
                   const runtimeObj: Runtime = new PyodideRuntime(runtime.src, runtime.name, runtime.lang); 
        
                   const script = document.createElement('script'); // create a script DOM node 
        
                   script.src = runtimeObj.src; // set its src to the provided URL 
        
                   script.addEventListener('load', () => { 
        
                       void runtimeObj.initialize(); 
        
                   }); 
        
                   document.head.appendChild(script); 
        
               } 
        
           }

Once the script has been loaded, the load event is fired and we call runtimeObj.initialize()

runtimeObj.initialize() is this, and at the end it prints PyScript page fully initialized:

pyscript/pyscriptjs/src/runtime.ts

Lines 135 to 181 in 4694602

    
           async initialize(): Promise<void> { 
        
               loader?.log('Loading runtime...'); 
        
               await this.loadInterpreter(); 
        
               const newEnv = { 
        
                   id: 'default', 
        
                   runtime: this, 
        
                   state: 'loading', 
        
               }; 
        
               runtimeLoaded.set(this); 
        
               // Inject the loader into the runtime namespace 
        
               // eslint-disable-next-line 
        
               this.globals.set('pyscript_loader', loader); 
        
               loader?.log('Runtime created...'); 
        
               loadedEnvironments.update(environments => ({ 
        
                   ...environments, 
        
                   [newEnv['id']]: newEnv, 
        
               })); 
        
               // now we call all initializers before we actually executed all page scripts 
        
               loader?.log('Initializing components...'); 
        
               for (const initializer of initializers_) { 
        
                   await initializer(); 
        
               } 
        
               loader?.log('Initializing scripts...'); 
        
               for (const script of scriptsQueue_) { 
        
                   await script.evaluate(); 
        
               } 
        
               scriptsQueue.set([]); 
        
               // now we call all post initializers AFTER we actually executed all page scripts 
        
               loader?.log('Running post initializers...'); 
        
               if (appConfig_ && appConfig_.autoclose_loader) { 
        
                   loader?.close(); 
        
               } 
        
               for (const initializer of postInitializers_) { 
        
                   await initializer(); 
        
               } 
        
               // NOTE: this message is used by integration tests to know that 
        
               // pyscript initialization has complete. If you change it, you need to 
        
               // change it also in tests/integration/support.py 
        
               logger.info('PyScript page fully initialized'); 
        
           }

So, I don't really understand how it is possible that (4) is called before the <script> is added to the head. I also tried to put a console.log after document.head.appendChild(script) and I confirm that it's printed way before PyScript page fully initialized.

So, there must be another reason for why the time.sleep is needed, but I think this investigation should not be a blocker to merge this PR, as it contains a lot of good stuff.

Moreover, I plan to do completely do a refactoring to improve the life cycle of a page and probably the code will change a lot, so maybe it's better to continue the investigation after the refactoring.

FabioRosado · 2022-09-06T15:31:01Z

Thank you for the review and the walkthrough! I was clearly wrong thinking that the appendChild step was the reason why we had to wait a short period of time 🤔

pyscript/pyscriptjs/src/components/pyconfig.ts

Line 71 in f3157b3

document.head.appendChild(script);

Although I should have added a log statement there, but I forgot 😞

So yeah, It's a mystery still why we need to wait this short time, I'll probably poke at it a bit more to see if I can figure out what's happening 😄

…ntent

antocuni · 2022-09-07T09:25:17Z

also, I see that currently the tests are failing because of test_d3. This test seems to be flaky, I have already seen it failing occasionally 😞

…fail flaky d3 test

antocuni

Thank you, looks good to me!
Another related issue is #763 which aims to refactor heavily the life cycle of a page, but I'm happy to merge this PR immediately.
Thank you!

…tests (pyscript#747) * Create custom elements when the runtime finishes loading * Remove xfails and fix repl integration test * Fix commented ignore * Address Antonio's comments * Fix bad rebase * Make ure to wait for repl to be in attached state before asserting content * Move createCustomeElement up so it runs before we close the loader, xfail flaky d3 test * Fix xfail

* Execute pys-on* events when triggered, not at load Mimicing the behavior of Javascripts 'onLoad' event, we should not be executing the use code at page-load time, only when the event is triggered. * Update examples to new syntax * Fix merge issue * Await running event handler code * Restore pys-on* events with original behavior, deprecation warning * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * xfail toga example * Add missing { (typo) * Adjust callback chandling to make linter happy * Change alpha to latest (#760) * Don't create custom elements in main and fix various small issues on tests (#747) * Create custom elements when the runtime finishes loading * Remove xfails and fix repl integration test * Fix commented ignore * Address Antonio's comments * Fix bad rebase * Make ure to wait for repl to be in attached state before asserting content * Move createCustomeElement up so it runs before we close the loader, xfail flaky d3 test * Fix xfail * [pre-commit.ci] pre-commit autoupdate (#762) updates: - [github.com/pre-commit/mirrors-eslint: v8.23.0 → v8.23.1](pre-commit/mirrors-eslint@v8.23.0...v8.23.1) Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> * change documentation to point to latest instead of frozen alpha (#764) * Toga example is xpass * Correct 'xpass' to 'xfail' mark Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Peter W <34256109+pww217@users.noreply.github.com> Co-authored-by: Fábio Rosado <fabiorosado@outlook.com>

As the title stays, the main goal of the branch is to kill the infamous runtimeLoaded global store and all the complications, problems and bugs caused by the fact that in many places we needed to ensure/wait that the global runtime was properly set before being able to execute code. The core idea is that runtime is never a global object and that it's passed around explicitly, which means that when a function receives it, it is guaranteed to be initialized&ready. This caused a bit of complications in pybutton.ts, pyinputbox.ts and pyrepl.ts, because they indirectly want to call runtime.run from connectedCallback, which is the only place where we cannot explicitly pass the runtime because it's automatically called by the browser. But also, it is also a sign of a bad design, because it were entirely possible that connectedCallback was called before the runtime was ready, which probably caused many bugs, see e.g. #673 and #747. The solution to is use dependency injection and create the class later on: so instead of having a global PyButton class which relies on a global runtime (whose state is uncertain) we have a make_PyButton function which takes a runtime and make a PyButton class which is tied to that specific runtime (whose state is certainly ready, because we call make_PyButton only when we know that the runtime is ready). Similar for PyInputBox and PyRepl. Other highlights: thanks to this, I could kill the also infamous runAfterRuntimeInitialized and a couple of smelly lines which used setTimeout to "wait" for the runtime. While I was at it, I also called a lot of other stores which were completely unused and where probably leftovers from a past universe.

As the title stays, the main goal of the branch is to kill the infamous runtimeLoaded global store and all the complications, problems and bugs caused by the fact that in many places we needed to ensure/wait that the global runtime was properly set before being able to execute code. The core idea is that runtime is never a global object and that it's passed around explicitly, which means that when a function receives it, it is guaranteed to be initialized&ready. This caused a bit of complications in pybutton.ts, pyinputbox.ts and pyrepl.ts, because they indirectly want to call runtime.run from connectedCallback, which is the only place where we cannot explicitly pass the runtime because it's automatically called by the browser. But also, it is also a sign of a bad design, because it were entirely possible that connectedCallback was called before the runtime was ready, which probably caused many bugs, see e.g. pyscript#673 and pyscript#747. The solution to is use dependency injection and create the class later on: so instead of having a global PyButton class which relies on a global runtime (whose state is uncertain) we have a make_PyButton function which takes a runtime and make a PyButton class which is tied to that specific runtime (whose state is certainly ready, because we call make_PyButton only when we know that the runtime is ready). Similar for PyInputBox and PyRepl. Other highlights: thanks to this, I could kill the also infamous runAfterRuntimeInitialized and a couple of smelly lines which used setTimeout to "wait" for the runtime. While I was at it, I also called a lot of other stores which were completely unused and where probably leftovers from a past universe.

FabioRosado changed the title ~~Fr/runtime undefined~~ Don't create custom elements in main and fix various small issues on tests Sep 4, 2022

antocuni requested changes Sep 6, 2022

View reviewed changes

Comment thread pyscriptjs/src/runtime.ts Outdated

Comment thread pyscriptjs/tests/integration/support.py

FabioRosado added 3 commits September 6, 2022 08:21

Create custom elements when the runtime finishes loading

2b7890d

Remove xfails and fix repl integration test

a0599ac

Fix commented ignore

b1208ce

Address Antonio's comments

29d0884

FabioRosado force-pushed the fr/runtime-undefined branch from e0d4c60 to 29d0884 Compare September 6, 2022 15:35

FabioRosado added 2 commits September 6, 2022 08:36

Fix bad rebase

080f564

Make ure to wait for repl to be in attached state before asserting co…

571688b

…ntent

antocuni reviewed Sep 7, 2022

View reviewed changes

Comment thread pyscriptjs/src/runtime.ts Outdated

antocuni mentioned this pull request Sep 7, 2022

Use Playwright's wait_for_selector to delay until Pyodide loads and renders #694

Closed

3 tasks

FabioRosado added 2 commits September 11, 2022 19:21

Move createCustomeElement up so it runs before we close the loader, x…

998766e

…fail flaky d3 test

Fix xfail

d3d20e7

marimeireles added the waiting on feedback Issue or PR waiting on feedback from core team label Sep 12, 2022

antocuni approved these changes Sep 13, 2022

View reviewed changes

antocuni merged commit c566977 into pyscript:main Sep 13, 2022

FabioRosado mentioned this pull request Sep 14, 2022

The integration test 'test_repl' is flaky #726

Closed

3 tasks

FabioRosado deleted the fr/runtime-undefined branch September 14, 2022 09:22

JeffersGlass mentioned this pull request Sep 14, 2022

Execute pys-on* events when triggered, not at load #686

Merged

This was referenced Sep 26, 2022

Add more unit and integration tests #773

Merged

Refactor py-config and the general initialization logic of the page #806

Merged

antocuni mentioned this pull request Oct 13, 2022

kill stores.runtimeLoaded and many other stores #850

Merged

FabioRosado mentioned this pull request Oct 28, 2022

Js TypeError: Cannot read properties of undefined (reading 'setAttribute') #677

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't create custom elements in main and fix various small issues on tests#747

Don't create custom elements in main and fix various small issues on tests#747
antocuni merged 8 commits into
pyscript:mainfrom
FabioRosado:fr/runtime-undefined

FabioRosado commented Sep 4, 2022

Uh oh!

antocuni left a comment

Uh oh!

Uh oh!

Uh oh!

FabioRosado commented Sep 6, 2022

Uh oh!

Uh oh!

antocuni commented Sep 7, 2022

Uh oh!

antocuni left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	appConfig.set(this.values);
	logger.info('config set:', this.values);

	this.loadRuntimes();
	}

	loadRuntimes() {
	logger.info('Initializing runtimes');
	for (const runtime of this.values.runtimes) {
	const runtimeObj: Runtime = new PyodideRuntime(runtime.src, runtime.name, runtime.lang);
	const script = document.createElement('script'); // create a script DOM node
	script.src = runtimeObj.src; // set its src to the provided URL
	script.addEventListener('load', () => {
	void runtimeObj.initialize();
	});
	document.head.appendChild(script);
	}
	}

	async initialize(): Promise<void> {
	loader?.log('Loading runtime...');
	await this.loadInterpreter();
	const newEnv = {
	id: 'default',
	runtime: this,
	state: 'loading',
	};
	runtimeLoaded.set(this);

	// Inject the loader into the runtime namespace
	// eslint-disable-next-line
	this.globals.set('pyscript_loader', loader);

	loader?.log('Runtime created...');
	loadedEnvironments.update(environments => ({
	...environments,
	[newEnv['id']]: newEnv,
	}));

	// now we call all initializers before we actually executed all page scripts
	loader?.log('Initializing components...');
	for (const initializer of initializers_) {
	await initializer();
	}

	loader?.log('Initializing scripts...');
	for (const script of scriptsQueue_) {
	await script.evaluate();
	}
	scriptsQueue.set([]);

	// now we call all post initializers AFTER we actually executed all page scripts
	loader?.log('Running post initializers...');

	if (appConfig_ && appConfig_.autoclose_loader) {
	loader?.close();
	}

	for (const initializer of postInitializers_) {
	await initializer();
	}
	// NOTE: this message is used by integration tests to know that
	// pyscript initialization has complete. If you change it, you need to
	// change it also in tests/integration/support.py
	logger.info('PyScript page fully initialized');
	}

Conversation

FabioRosado commented Sep 4, 2022

runAfterRuntimeInitialized running immediately on page load

Removing the time.sleep from tests

Uh oh!

antocuni left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

FabioRosado commented Sep 6, 2022

Uh oh!

Uh oh!

antocuni commented Sep 7, 2022

Uh oh!

antocuni left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants