-
Notifications
You must be signed in to change notification settings - Fork 22
Implement mechanism to serve files via custom protocols #21
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
Conversation
1bc1b2c
to
88e5aed
Compare
You will need to add some tests that can be run on the CI. |
|
||
/** | ||
* Materializes virtual files in a temporary directory and links to them | ||
* via file:// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment should be formatted as follows.
/** Materializes virtual files in a temporary directory and links to them
* via file://
*/
The code logic looks good to me, but we really need to test it on the CI. |
Not sure how to test the feature exactly. I added a new target seleniumJSHttpEnv using the SpecificFileMaterializer and running one test which makes sure the script is served over HTTP. I noticed there is no '.idea/' in the .gitignore, shall this be added ? |
892f8d2
to
a95ab34
Compare
No. You should add that to your global |
|
||
private val materializer = new VirtualFileMaterializer(true) | ||
|
||
override def materialize(vf: VirtualTextFile): Unit = materializer.materialize(vf) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think, but needs verification, that it would be better if this method returned a URL
, and getAbsolutePath
was removed.
a95ab34
to
60c805a
Compare
- sbt ++$TRAVIS_SCALA_VERSION seleniumJSEnvTest/run seleniumJSEnvTest/test 'set scalaJSStage in Global := FullOptStage' seleniumJSEnvTest/run seleniumJSEnvTest/test | ||
- sbt ++$TRAVIS_SCALA_VERSION 'set inScope(ThisScope in seleniumJSEnvTest)(jsEnv := new org.scalajs.jsenv.selenium.SeleniumJSEnv(org.scalajs.jsenv.selenium.Firefox).withKeepAlive())' seleniumJSEnvTest/run seleniumJSEnvTest/test 'set scalaJSStage in Global := FullOptStage' seleniumJSEnvTest/run seleniumJSEnvTest/test | ||
- sbt ++$TRAVIS_SCALA_VERSION seleniumJSHttpEnvTest/test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should add fullOpt
to the tests:
- sbt ++$TRAVIS_SCALA_VERSION seleniumJSHttpEnvTest/test 'set scalaJSStage in Global := FullOptStage' seleniumJSHttpEnvTest/test
That is all. |
Could you also change the commit message to |
60c805a
to
8ec743d
Compare
All done, thanks for the review. |
LGTM |
In some scenarios, one may need the runtime files to be served over http:// instead of file:// (ie. to bypass cross origin request security).
In this first attempt, I created a new trait for file materializer which also provide an absolute access url for materialized files. This allows to encapsulate the "absolutePath" function together with the materializer. This trait has two direct implementations. One which is a refactoring of the current code (for the scenario where you don't care how selenium gets its runtime files, you just want it to work). The other one allows to customize the location where files get saved and the actual URL used by selenium to access them.
At usage it is pretty nice:
The main problem I see with this approach is that my FileMaterializer trait is pretty redundant with respect to the VirtualFileMaterializer class. I suppose a cleaner approach would be to push all of this logic upstream such that VirtualFileMaterializer is the trait with two children and it exposes the getAbsolutePath method.