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

spawn method lack of arguments if no spawnProcess module set #1032

Closed
mho22 opened this issue Feb 12, 2024 · 9 comments
Closed

spawn method lack of arguments if no spawnProcess module set #1032

mho22 opened this issue Feb 12, 2024 · 9 comments

Comments

@mho22
Copy link
Contributor

mho22 commented Feb 12, 2024

@adamziel I apologize. It seems that I forgot to inject the args parameter into the spawn() method when no setSpawnHandler method has been set.

packages/php-wasm/compile/php/phpwasm-emscripten-library.js on line 198:

if (ENVIRONMENT_IS_NODE) {
    return require('child_process').spawn(command, [], {
	shell: true,
	stdio: ['pipe', 'pipe', 'pipe'],
	timeout: 100,
    });
}

should be :

...
    return require('child_process').spawn(command, args, {
...

Do you confirm the new Pull Request ?

@adamziel
Copy link
Collaborator

That's the default Node.js handler. Yes, it would be nice to pass args there as well – would you be up for starting a PR for that?

@mho22
Copy link
Contributor Author

mho22 commented Feb 13, 2024

@adamziel Yep! I am on it.

@mho22
Copy link
Contributor Author

mho22 commented Feb 13, 2024

@adamziel The pull request is ready to be merged ! 🎉

BTW, I was trying to make the setSpawnHandler method work inside a javascript file following the steps found in the php.spec.ts file [ using createSpawnHandler method ] :

test.js :

import { NodePHP } from '@php-wasm/node';
import { createSpawnHandler } from '@php-wasm/util';


const php = await NodePHP.load( environment.php );

const handler = createSpawnHandler( ( command, processApi ) => {
	processApi.on( 'stdin', data => { processApi.stdout( data ); });
	processApi.flushStdin();
	processApi.exit( 0 );
} );

php.setSpawnHandler( handler );

const result = await php.cli( [ 'php', '-r',

    `$command = "echo 'Hello world!'";

    $descriptorspec = [
        1 => [ "pipe", "w" ],
        2 => [ "pipe", "w" ]
    ];

    $res = proc_open( $command, $descriptorspec, $pipes );

    $stdout = stream_get_contents($pipes[1]);

    proc_close($res);

    echo $stdout;`

 ] );

console.log( result );

process.exit( 0 );
node test.js

But this code did not want work. I couldn't access createSpawnHandler. Is this intended ?

adamziel pushed a commit that referenced this issue Feb 14, 2024
Based on the issue #1032 

`packages/php-wasm/compile/php/phpwasm-emscripten-library.js` on line
`198`:

```javascript
if (ENVIRONMENT_IS_NODE) {
    return require('child_process').spawn(command, [], {
	shell: true,
	stdio: ['pipe', 'pipe', 'pipe'],
	timeout: 100,
    });
}
```

The `spawn` method should take `args` into account instead of an empty
array.

```javascript
...
    return require('child_process').spawn(command, args, {
...
```
@adamziel adamziel added this to the PHP Feature Parity milestone Feb 29, 2024
@adamziel
Copy link
Collaborator

adamziel commented Mar 4, 2024

I couldn't access createSpawnHandler. Is this intended ?

What was the error?

@mho22
Copy link
Contributor Author

mho22 commented Mar 4, 2024

I got this :

php-wasm  node test.js
file:///php-wasm/test.js:2
import { createSpawnHandler } from '@php-wasm/util';
         ^^^^^^^^^^^^^^^^^^
SyntaxError: Named export 'createSpawnHandler' not found. The requested module '@php-wasm/util' is a CommonJS module, which may not support all module.exports as named exports.

Node.js v21.6.0

@adamziel
Copy link
Collaborator

adamziel commented Mar 4, 2024

That's unexpected, the export is present:

export { createSpawnHandler } from './create-spawn-handler';

Perhaps that's an issue with paths 🤔 I wonder if it resolves the correct module. What would it say if you import everything from '@php-wasm.util'; console.log(everything);?

@mho22
Copy link
Contributor Author

mho22 commented Mar 4, 2024

I have this :

test.js

(node:25098) Warning: To load an ES module, set "type": "module" in the package.json or use the .mjs extension.
php-wasm/node_modules/@php-wasm/util/index.js:290
export {
^^^^^^

SyntaxError: Unexpected token 'export'
    at internalCompileFunction (node:internal/vm:77:18)
    at wrapSafe (node:internal/modules/cjs/loader:1290:20)
    at Module._compile (node:internal/modules/cjs/loader:1342:27)
    at Module._extensions..js (node:internal/modules/cjs/loader:1437:10)
    at Module.load (node:internal/modules/cjs/loader:1212:32)
    at Module._load (node:internal/modules/cjs/loader:1028:12)
    at cjsLoader (node:internal/modules/esm/translators:359:17)
    at ModuleWrap.<anonymous> (node:internal/modules/esm/translators:308:7)
    at ModuleJob.run (node:internal/modules/esm/module_job:218:25)
    at async ModuleLoader.import (node:internal/modules/esm/loader:323:24)

Node.js v21.6.0

But it seems like @php-wasm/util/package.json type equals commonjs that's probably why I couldn't make this work.

If I change this in @php-wasm/util/package.json :

- "type": "commonjs",
+ "type": "module",
+ "main": "index.js",

It works as I expected it, but this is probably not what you expect on your side.

adamziel added a commit that referenced this issue Mar 5, 2024
Solves #1032 where the package cannot be `require`-d in node.js
adamziel added a commit that referenced this issue Mar 5, 2024
Solves #1032 where the package cannot be `require`-d in node.js

Thank you for reporting @mho22 !
@adamziel
Copy link
Collaborator

adamziel commented Mar 5, 2024

@mho22 #1081 should resolve it. I just published new NPM packages – I'm going to close this issue. Please confirm it worked for you, and if not then let's reopen.

@adamziel adamziel closed this as completed Mar 5, 2024
@mho22
Copy link
Contributor Author

mho22 commented Mar 5, 2024

@adamziel It is working now with version 0.6.5. I am glad I could help with this issue too 🎉 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants