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

PHP: Add listFiles options to IsomorphicLocalPHP and WebPHPEndpoint #490

Merged
merged 3 commits into from
Jun 1, 2023

Conversation

adamziel
Copy link
Collaborator

@adamziel adamziel commented Jun 1, 2023

The options argument for listFiles was added to BasePHP in #462. This commit adds it to an interface and a Web PHP Endpoint so that it can be used across the board in web and node.js applications

To test, confirm the CI is green.

Note for the future: Ideally something would flash red in the CI if BasePHP interface is updated without adjusting the other to. I completely forgot about them.

cc @eliot-akira

The options argument for listFiles was added to BasePHP in #462. This
commit adds it to an interface and a Web PHP Endpoint so that it can be
used across the board in web and node.js applications
@@ -31,15 +31,17 @@ export async function installAsset(
// Extract to temporary folder so we can find asset folder name

const zipFileName = zipFile.name;
const tmpFolder = `/tmp/assets`;
const assetNameGuess = zipFileName.replace(/\.zip$/, '');
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh this wasn't meant for this PR at all, let's clean it up

@adamziel adamziel merged commit e94e919 into trunk Jun 1, 2023
@adamziel adamziel deleted the add-listfiles-options-to-interface branch June 1, 2023 16:06
@eliot-akira
Copy link
Collaborator

Ah I see, when BasePHP was updated, the new method signature needed to be reflected in IsomorphicLocalPHP and WebPHPEndpoint.

That reminds me, I was trying to get access to BasePHP.addServerGlobalEntry, so I can set server constants to $_SERVER from the Playground client. To expose this method as part of the public API, I found several places that (may) need to be updated: WebPHPEndpoint, IsomorphicLocalPHP, PHPBrowser, and PHPRequestHandler. The clue that I followed was where the method pathToInternalUrl and internalUrlToPath were being defined. In the end, I couldn't get addServerGlobalEntry to remember the properties across PHP requests, it seems they're freshly defined every time.

@adamziel
Copy link
Collaborator Author

adamziel commented Jun 2, 2023

Yes, addServerGlobalEntry is per-request. Would you share more why it would be useful for you to retain it across many requests? And also the amount of types to update is quite overwhelming, it would be nice to at least ensure TypeScript tells you about them, or, ideally, only have a single location to change.

@eliot-akira
Copy link
Collaborator

addServerGlobalEntry is per-request

I see, I was trying to use it to pass some constants for a plugin running on the embedded site, so it can detect that it's within a custom Playground environment. It would have been convenient to have a way to persist some values across requests, but upon reflection, it makes sense the $_SERVER variable needs to be freshly populated, since in PHP the universe is created and destroyed on every request.

I'm still figuring out a comfortable way to communicate between the Playground host and the code running on the embedded website. So far I've been using a queue of messages in the database to send and receive data/actions.

amount of types to update.. ensure TypeScript tells you about them, or, ideally, only have a single location to change

The number of places where pathToInternalUrl and internalUrlToPath were defined, it was difficult to understand the class inheritance and subtle differences between each definition. I'm guessing part of the complexity is a translation layer that converts method calls into worker requests.

Anyway, I'm enjoying getting familiar with the code, learning the logic and thoughts behind it.

@adamziel
Copy link
Collaborator Author

adamziel commented Jun 3, 2023

I'm still figuring out a comfortable way to communicate between the Playground host and the code running on the embedded website.

Would the post_message_to_js api fully solve it for you? Or would it still leave an unaddressed need or two? (We could follow up in that PR)

@adamziel
Copy link
Collaborator Author

adamziel commented Jun 3, 2023

@eliot-akira I just opened #514 to keep track of the complexity issue

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

Successfully merging this pull request may close these issues.

2 participants