Allow pyscript package to contain multiple files #1262
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Followup to pyscript#1232. Closes pyscript#1226. Use node to make a manifest of the src/python dir and then use terser to inject it into the bundle as a variable called pyscript_package. This means we need to always use the terser plugin even when not minifying. In the non-minify case, we disable terser minification and mangling and enable terser beautification. Note that we bundle mangled versions of many upstream npm dependencies, so even in debug/nonminified builds, these do not include symbol names.
7195b0e to
1e34b4b
Compare
|
Oh we seem to vendor mangled npm dependencies. This is not very good. |
hoodmane
added a commit
to hoodmane/pyscript
that referenced
this pull request
Mar 8, 2023
For mysterious reasons, these errors appear on my branch pyscript#1262 even though they are not related to changes there. The eslint config seems a bit unstable. Anyways this fixes them.
For mysterious reasons, these errors appear on my branch pyscript#1262 even though they are not related to changes there. The eslint config seems a bit unstable. Anyways this fixes them.
Merged
b8ab0cd to
7957814
Compare
antocuni
pushed a commit
that referenced
this pull request
Mar 13, 2023
* Unvendor toml package * Fix many ESlint errors For mysterious reasons, these errors appear on my branch #1262 even though they are not related to changes there. The eslint config seems a bit unstable. Anyways this fixes them. * Put back Record * Fix typescript compilation * Fix lints * Try @iarna/toml instead * Fix import * Use @ltd/j-toml * Update test * Use toml-j0.4 * Some changes * Fix toml import * Try adding eslint gha job * Add forgotten checkout action * Force CI to run * Blah * Fix * Revert changes to github workflow * Fix lints * wget toml-j0.4 type definitions * Add toml-j types workaround to eslint workflow * Apply formatter * Use @hoodmane/toml-j0.4 * Import from @hoodmane/toml-j0.4
There was a problem hiding this comment.
Thanks for the PR @hoodmane.
I've learned a bit of TS reviewing it, cool!
Most of my comments are questions of things that are not clear to me.
Once you rebase it it's good to me to merge.
| @@ -163,15 +166,15 @@ const pyAttributeToEvent: Map<string, string> = new Map<string, string>([ | |||
| ]); | |||
|
|
|||
| /** Initialize all elements with py-* handlers attributes */ | |||
| export function initHandlers(interpreter: InterpreterClient) { | |||
| export async function initHandlers(interpreter: InterpreterClient) { | |||
There was a problem hiding this comment.
Why are these now async?
There was a problem hiding this comment.
Sorry these changes are unrelated...
| plugins: [terser()], | ||
| }, | ||
| ], | ||
| output: [{ minify: true }, { minify: false }].map(({ minify }) => ({ |
There was a problem hiding this comment.
Where do I pass the minify argument to this? (in general, just because I don't really know what's calling this code)
There was a problem hiding this comment.
This needs to change because of #1298.
| interpreter._remote.interface.FS.mkdirTree('/home/pyodide/pyscript'); | ||
| interpreter._remote.interface.FS.writeFile('pyscript/__init__.py', pyscript); | ||
| // Write pyscript package into file system | ||
| for (let dir of pyscript_package.dirs) { | 67ED
There was a problem hiding this comment.
Do you think it makes sense to test this code?
|
Thanks for the review @marimeireles! |
|
Closed in favor of #1309. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Followup to #1232. Closes #1226.
Use node to make a manifest of the src/python dir and then use terser to inject it into the bundle as a variable called pyscript_package. This means we need to always use the terser plugin even when not minifying. In the non-minify case, we disable terser minification and mangling and enable terser beautification. Note that we bundle mangled versions of many upstream npm dependencies, so even in debug/nonminified builds, these do not include symbol names.