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

Add support for include() in remote files; remove need for proxy. #239

Merged

Conversation

jcoleman
Copy link
Contributor

Download all files directly (rather than tunneling through a proxy
server) to remove dependence on running a special server. This is
useful, for example, when providing a remote link to a Github repository
for a project that isn't a single file. Remote projects (with includes)
are currently limited to someone running a special server; this ensures
effectively any remote file host is usable instead.

We also store the include file base path separately now since it should
be resolved to the host of the primary file rather than the host serving
up the OpenJSCAD viewer.

This also cleans up the include logic a bit: currently regex parsing
is used for replacement which isn't safe: i.e., it's possible for the
regex to match unnecessarily. It's also the case that multiple includes
for the same file can fail since the current logic naively inserts the
included source multiple times. Instead I now ensure that the included
source is only executed once and rather than inline it override the
include function to do the work for me. Regex matching of what files
to load is still in place; ideally that'd be replaced as well since
regex parsing isn't safe.

Download all files directly (rather than tunneling through a proxy
server) to remove dependence on running a special server. This is
useful, for example, when providing a remote link to a Github repository
for a project that isn't a single file. Remote projects (with includes)
are currently limited to someone running a special server; this ensures
effectively any remote file host is usable instead.

We also store the include file base path separately now since it should
be resolved to the host of the primary file rather than the host serving
up the OpenJSCAD viewer.

This also cleans up the `include` logic a bit: currently regex parsing
is used for replacement which isn't safe: i.e., it's possible for the
regex to match unnecessarily. It's also the case that multiple includes
for the same file can fail since the current logic naively inserts the
included source multiple times. Instead I now ensure that the included
source is only executed once and rather than inline it override the
`include` function to do the work for me. Regex matching of what files
to load is still in place; ideally that'd be replaced as well since
regex parsing isn't safe.
@jcoleman jcoleman force-pushed the fix-include-for-remote-files-rebased-on-e272ba3 branch from 91ad9f7 to 4a05755 Compare April 17, 2017 16:04
@z3dev
Copy link
Member

z3dev commented Apr 17, 2017

@jcoleman Thanks for this contribution. This looks like an awesome solution to the ugly implementation of include().

Obviously, we need to test the include of files from various sources. It seems that you've been studying this. :)

Can you give us some scripts for testing the new include functionality? Here's what we have now.

  • Load index.html from server, open Example for Include: Globe and Recursive Include: Platronics
  • Select multiple files with include, and drop to page
  • Select folder of files, and drop to page

@jcoleman
Copy link
Contributor Author

@z3dev I made this fix to support a project of mine: https://github.com/jcoleman/ada-keyboard

In particular, here's a link that exercises the new code (using my fork of the viewer): https://jcoleman.github.io/OpenJSCAD.org/#https://raw.githubusercontent.com/jcoleman/ada-keyboard/master/case/main.jscad

Is that what you're looking for?

@kaosat-dev
Copy link
Contributor

@jcoleman Thanks for this PR !
Very nice overall ! kudos !
One main problem : I also have this Pr #241 that touches exactly the same areas, with lots of overlaps.
I also ditched the regexp and switched to a much more robust ast based approach.

Since I have a lot more fixes in that PR , would you be ok with us merging #241 first, and then I can handle merge conflicts with your PR on top of that ?

@kaosat-dev
Copy link
Contributor

A few more notes also @jcoleman :

  • I love what you did with the examples
  • I believe we need a default for includePathBaseUrl for the other cases
  • Overall , the whole 'include' is a giant pain, I have a proposal to switch to an actual 'require()' based system, as we are reinventing the wheel more and more, and badly

@jcoleman
Copy link
Contributor Author

@kaosat-dev Merging the other works first; is there a timeline on getting that and then this fix in?

Defaulting includePathBaseUrl to the existing baseurl I think should work fine; in fact I think I had that in my original work, but in the two rebases I've done prior to this PR (my initial work was on the release version) that might have gotten lost.

I'm curious why include was switched to the regex version instead of overriding the function as it works in the existing release version. Prefetching is of course a benefit, but I wondered if there was any other reason since solely patching the function is a lot easier to reason about than trying to parse the JS to find all files that need to be included.

One other thing: I assume when this gets merged probably the proxy server code can be deleted; I didn't do that because I wanted to have others verify the stability of this change first.

@kaosat-dev
Copy link
Contributor

@jcoleman I just finished the work on that PR , so I think it can be merged tomorrow at the latest ?
Really sorry about this !

Yeah if it uses the existing baseurl it should be fine, I'll take your PR for a spin to see if anything breaks: it is just an area of Jscad that is kinda finicky, and I found a few extra edge cases again this morning , haha :)

I would need to double check when I am not half asleep, but If I recall right there were multiple aspects to the switch of include :

  • since fetching the resolved code is async you cannot block until the contents are fetched (so not possible to use include as a simple function given the current syntax in an easy manner, hence why the basic include is a no-op that gets replaced)
  • there were some really bad window globals pollutions happening but honestly I might be conflating with an other issue in the same 'area' of code
    Side note, the new AST based find / replace is MUCH more robust, and could bring some nice perks for the future.

About the proxy: we can double check and remove stuff stepwise, but less code is always better :)

@kaosat-dev
Copy link
Contributor

@jcoleman The other PR is now merged, will try to combine your changes

@kaosat-dev kaosat-dev mentioned this pull request Apr 21, 2017
@kaosat-dev
Copy link
Contributor

@jcoleman I had to open another PR here #244 to merge things because there were a few things to fix/change to your original PR (next time I'll try to merge back to the original PR if possible).

By the way , pretty please make sure the examples and command line unit test run before submitting the PR :)

@jcoleman
Copy link
Contributor Author

@kaosat-dev Yep, I did run tests/examples on the first iteration (a few months ago, actually), but I kinda got discouraged with the massive amount of change between master and dev and just wanted to get it working this time. (Excuses!)

@kaosat-dev
Copy link
Contributor

@jcoleman haha I understand, that should really get better soon, once we FINALY merge dev into master, things will be more stable.

@z3dev z3dev merged commit 4a05755 into jscad:dev Apr 21, 2017
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

Successfully merging this pull request may close these issues.

4 participants