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

Node should serve files directly to puppeteer #97

Closed
samreid opened this issue May 27, 2020 · 10 comments
Closed

Node should serve files directly to puppeteer #97

samreid opened this issue May 27, 2020 · 10 comments
Assignees

Comments

@samreid
Copy link
Member

samreid commented May 27, 2020

From #90

Having a hardcoded slash after the localTestingURL was causing bugs for me on Windows http-server, and this was not how we used localTestingURL when generating PhET-iO API files. I removed that hard coded slash, and added support for dynamically adding it if it isn't provided from the client's build-local.

Another way to address this would be to have node serve the files to puppeteer instead of relying on the developer localhost. We implemented this for https://github.com/phetsims/phet-io/issues/1664 and the same should be possible here.

@samreid samreid self-assigned this May 27, 2020
@samreid
Copy link
Member Author

samreid commented May 27, 2020

Let's try to factor out common code between this and phetsims/chipper@9d74fe9

@samreid
Copy link
Member Author

samreid commented Jul 11, 2020

The usages of localTestingURL in aqua/Gruntfile.js fall more under the CT umbrella, and I can't tell whether it would be appropriate to use http.createServer in those cases. @jonathanolson what do you recommend?

@samreid
Copy link
Member Author

samreid commented Jul 11, 2020

The other part for aqua is in

await page.goto( `${targetURL}&qunitHooks` );

@zepumph
Copy link
Member

zepumph commented Jul 12, 2021

In phetsims/perennial#175 (comment) I was surprised that I found that when using http.createServer to load a sim in binder, a bug in supplying the MIME type (Content-Type header) broke the sim. Why does this not occur with the Continuous Server? perhaps because I was specifying an svg incorrectly as a plain/text. Do you think this server could benefit from using withServer? Or does it need to exist longer than that function is set up to?

@zepumph
Copy link
Member

zepumph commented Jul 12, 2021

In regards to #97 (comment), this no longer uses localTestingURL because of its usage change over in #97 (comment) in hook-pre-commit to use withServer, so I think this is really just about ContinuousServer.js

@jonathanolson
Copy link
Contributor

The usages of localTestingURL in aqua/Gruntfile.js fall more under the CT umbrella, and I can't tell whether it would be appropriate to use http.createServer in those cases. @jonathanolson what do you recommend?

Seems possible to serve that way, but would it be a performance hit? Seems nice to factor it out so it's not needed if possible.

@samreid
Copy link
Member Author

samreid commented Jul 12, 2021

I'm not very familiar with it, but it seems that ContinuousServer is supposed to last longer than the withServer is designed for. @zepumph what do you recommend?

@samreid samreid assigned zepumph and unassigned samreid Jul 12, 2021
@zepumph
Copy link
Member

zepumph commented Jul 12, 2021

ContinuousServer.startServer begins and runs until the program stops. Sounds like we should leave it as is. I think this issue is outdated and should be closed.

@samreid
Copy link
Member Author

samreid commented Jul 12, 2021

Closing sounds good to me.

@zepumph
Copy link
Member

zepumph commented Jun 9, 2022

I also want to add that we now have a puppeteer-based client for CT, see #143

@zepumph zepumph closed this as completed Jun 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants