Skip to content

[PHP.wasm for Node] Fix php.js import path in the published npm package#1958

Merged
adamziel merged 2 commits intotrunkfrom
fix-php-path
Oct 29, 2024
Merged

[PHP.wasm for Node] Fix php.js import path in the published npm package#1958
adamziel merged 2 commits intotrunkfrom
fix-php-path

Conversation

@psrpinto
Copy link
Copy Markdown
Member

@psrpinto psrpinto commented Oct 29, 2024

@sejas has brought to our attention that the imports of the wasm assets are using incorrect paths, since 3bebb24.

The source says something like:

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

Which currently results in the built file saying:

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:

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

Testing instructions

Build the package:

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.

@psrpinto psrpinto self-assigned this Oct 29, 2024
@psrpinto psrpinto added [Type] Bug An existing feature does not function as intended [Package][@php-wasm] Web labels Oct 29, 2024
@psrpinto psrpinto marked this pull request as ready for review October 29, 2024 12:51
@psrpinto psrpinto requested a review from adamziel October 29, 2024 12:51
@adamziel adamziel changed the title [php-wasm] Fix paths to wasm assets [PHP.wasm for Node] Fix php.js import path in the published npm package Oct 29, 2024
@adamziel adamziel merged commit 089bfdc into trunk Oct 29, 2024
@adamziel adamziel deleted the fix-php-path branch October 29, 2024 20:38
@adamziel
Copy link
Copy Markdown
Collaborator

Lovely fix, thank you @psrpinto! My only note is around the PR title – I adjusted it to make it summarize the entire PR. It's useful to have a full information right on the list when searching for a faulty commit.

@sejas
Copy link
Copy Markdown
Collaborator

sejas commented Oct 30, 2024

@psrpinto , Thank you for the quick fix! 🙏

@psrpinto
Copy link
Copy Markdown
Member Author

Thanks for renaming the PR title @adamziel, I agree your version is much more explicit.

I adjusted it to make it summarize the entire PR

I will make sure to follow this reasoning in future PRs I open 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Package][@php-wasm] Web [Type] Bug An existing feature does not function as intended

Projects

No open projects
Archived in project

Development

Successfully merging this pull request may close these issues.

3 participants