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

Use http.createServer instead of localTestingURL #175

Closed
5 tasks done
zepumph opened this issue May 27, 2020 · 10 comments
Closed
5 tasks done

Use http.createServer instead of localTestingURL #175

zepumph opened this issue May 27, 2020 · 10 comments

Comments

@zepumph
Copy link
Member

zepumph commented May 27, 2020

After seeing some great work done over in https://github.com/phetsims/phet-io/issues/1664 by @samreid, it is clear and simple to create a server instead of relying on localTestingURL. For example see this commit here: phetsims/chipper@9d74fe9.

I think we should try to get rid of this everywhere. Here is where I see usages currently:

Assigning to @samreid also in case he has opinions.

@samreid
Copy link
Member

samreid commented May 27, 2020

I agree we should let node serve files and get away from localTestingURL as much as possible. I opened phetsims/aqua#97 for the qunit part.

@samreid
Copy link
Member

samreid commented Jun 24, 2020

localTestingURL is now used in a sublime plugin:

def get_local_testing_url():
  return get_build_local()[ 'localTestingURL' ]

// ...

class PhetOpenPhetmarksCommand(sublime_plugin.TextCommand):
  def run(self, edit):
    view = self.view
    open_link(get_local_testing_url() + 'phetmarks')

Hence we may not be able to delete it. But we should still get rid of as many usages as possible.

@samreid samreid changed the title Get rid of localTestingURL Use http.createServer instead of localTestingURL Jul 11, 2020
@samreid
Copy link
Member

samreid commented Aug 19, 2020

I'm not planning to work on this in the near future, unassigning.

@zepumph
Copy link
Member Author

zepumph commented Jul 12, 2021

@jonathanolson created withServer in perennial which is pretty excellent. I would like to use it, or a copy of it, whenever possible, but I have a few concerns. It uses express, and I would rather do something that is more vanilla, so that we don't need to depend on such a heavy-weight server app across our project. For example, if I want to use this script in binder/ (for the issue found in #229), I don't want express leaking throughout our project.

The vanilla server used by PhET-iO API generation seems a bit more straight forward. I would like to try to use that instead. Tagging @jonathanolson about the work (in my working copy to be committed soon).

@zepumph
Copy link
Member Author

zepumph commented Jul 12, 2021

I could not figure out an easy way to test ReleaseBranch, so I converted the usage in Binder to confirm that the conversion works. I found that the mime types in the PhET-iO api generation are not complete for phet sims, and I had to add svg and json in order for the sim to fully load in puppeteer.

I didn't rerun generation though to make sure that new commits in #229 are working as expected.

@zepumph
Copy link
Member Author

zepumph commented Jul 12, 2021

I was able to use PERENNIAL/withServer for binder's usage above. That works really well, but we won't be able to use perennial in Chipper for generatePhetioMacroAPI. Perhaps that could benefit from a dual/ strategy for withServer.

@zepumph
Copy link
Member Author

zepumph commented Jul 12, 2021

PDOM Comparison went smoothly as well.

zepumph added a commit that referenced this issue Jul 12, 2021
zepumph added a commit that referenced this issue Jul 12, 2021
zepumph added a commit to phetsims/aqua that referenced this issue Jul 12, 2021
zepumph added a commit to phetsims/binder that referenced this issue Jul 12, 2021
zepumph added a commit to phetsims/phet-info that referenced this issue Jul 12, 2021
zepumph added a commit that referenced this issue Jul 12, 2021
zepumph added a commit to phetsims/binder that referenced this issue Jul 12, 2021
zepumph added a commit that referenced this issue Jul 12, 2021
@zepumph
Copy link
Member Author

zepumph commented Jul 12, 2021

Alright, all usages of localTestingURL have been removed expect that used by the phet-sublime-plugin. @jonathanolson over to you to review the above commits, and to update the python code for the plugin.

My goal is to tell people that they can remove that key from their build-local, so let me know when you think it is safe for me to do that.

@jonathanolson
Copy link
Contributor

Looks good to me! I'll change the plugin at my own convenience.

Looks safe to tell people that!

@zepumph
Copy link
Member Author

zepumph commented Jul 12, 2021

Posted, closing

localTestingURL is no longer used on the project. It can be removed from everyone's build-local.json files. This pattern was exchanged for just spinning up a simple, custom server whenever needed, see #175 for details.

14:30
The most dangerous usage conversion was in our pre-commit unit testing task. If you run into unexpected trouble there, please post in that issue. Thanks!

@zepumph zepumph closed this as completed Jul 12, 2021
samreid pushed a commit to phetsims/phet-info that referenced this issue Apr 23, 2022
samreid pushed a commit to phetsims/chipper that referenced this issue Aug 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants