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

fix vscode-extension bundle step adding correct jspi subfolder #359

Merged
merged 1 commit into from
Oct 25, 2024

Conversation

sejas
Copy link
Collaborator

@sejas sejas commented Oct 17, 2024

What?

It updates the correct path of jspi/PHP_* to be included in the VS Code extension after upgrading the Playground dependencies on #350

Why?

The paths to wasm files have changed in the new version.

How?

Updating the project bundle step.

Testing Instructions

  • Run npx nx run vscode-extension:build:bundle
  • Observe the command finishes successfully : > NX Successfully ran target build:bundle for project vscode-extension (501ms)

@sejas sejas self-assigned this Oct 17, 2024
@sejas
Copy link
Collaborator Author

sejas commented Oct 17, 2024

After upgrading the playground dependencies, the bundle steps for vscode-extension and interactive-code-block started failing.

In this PR I'm fixing the vscode-extension bundle step, but to fix interactive-code-block it seems we need to make the change in the wordpress-palyground repository. Here is the error I'm seeing:

web-php-jspi-wasm

It seems it was modified in this commit:
WordPress/wordpress-playground@3bebb24

@adamziel , do you know if it's a fail in the import or in the "build" process?
I'm seeing the import is correct on
https://github.com/WordPress/wordpress-playground/blob/2e48fa72a18ea222d697ff0602d3694f74b9162d/packages/php-wasm/web/src/lib/get-php-loader-module.ts#L22

So I'm wondering if we need to change the build process on the Playground repo to avoid having that php folder there.

@adamziel
Copy link
Collaborator

adamziel commented Oct 25, 2024

Good catch @sejas, @php-wasm/web needs a fix to produce the correct path (or flatten the php folder in the built package). CC @psrpinto – would you like to dive in and get some more exposure to Playground's build tooling?

@adamziel adamziel merged commit a671138 into trunk Oct 25, 2024
2 checks passed
@adamziel adamziel deleted the fix/jspi-package-builds branch October 25, 2024 11:26
@psrpinto
Copy link
Member

@adamziel Absolutely, I'd be happy to look into it. On it.

@psrpinto
Copy link
Member

I created a fix for the issue at WordPress/wordpress-playground#1958

adamziel pushed a commit to WordPress/wordpress-playground that referenced this pull request Oct 29, 2024
…ge (#1958)

@sejas has [brought to our
attention](WordPress/playground-tools#359 (comment))
that the imports of the wasm assets are using incorrect paths, since
3bebb24.

The
[source](https://github.com/WordPress/wordpress-playground/blob/trunk/packages/php-wasm/web/src/lib/get-php-loader-module.ts#L22)
says something like:

```ts
return await import('../../public/php/jspi/php_8_3.js');
```

Which currently results in the built file saying:

```js
return await import("./jspi/php_8_3.js");
```


However, since the tree of the published package is as follows, the path
in the built file is missing a leading `/php`.

```
php
├── asyncify
│   └── php_8_3.js
└── jspi
    └── php_8_3.js
```

This PR fixes the issue by making it so that the imports have a leading
`/php`, which results in the import being:

```js
return await import("./php/jspi/php_8_3.js");
```

## Testing instructions

Build the package:

```shell
npx nx run php-wasm-web:build
```

Then make sure that the paths in `dist/packages/php-wasm/web/index.js`
(around line 95) are correct, i.e. `./php/jspi/php_8_3.js`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants