Replace JSRender templating with vanilla js (resolves #30)

[anthonyhashemi]

Change:

Replace JSRender templating with vanilla js. Still need to import jsviews lib for the observable api, but that bit doesn't have the csp issue, only the views api/templating engine was a problem.

Reason:

See details of the CSP issue this is fixing here: #30

[anthonyhashemi]

@demiankatz I have tested this locally by spinning up examples/index.html and making sure that was consistent with master. Also, propagated this change up to universalviewer and then my project locally and the csp issue is gone and UV in my project acts the same as before.

It looks like the GH actions / checks on this repo are broken (same on another open PR here) so I don't know if someone has time to fix those or whether this would be able to be merged in without them working? Is there a specific testing process before PRs get merged in?

[demiankatz]

Thanks, @anthonyhashemi -- it looks like the build failure here is related to @edsilv's Netlify deployment. I'm not sure why this is broken or if it is still needed. I will talk to him about it and see if I can get a green checkmark, but we should be able to merge this even if the build is technically failing.

In any case, I'll also try to find time to review and test this before too long!

[demiankatz]

@anthonyhashemi, one thing that has me a little confused: where is JSRender even coming from in this project? I don't see it in the package.json or package-lock.json. Just wondering if there's some dependency we should remove now that this refactoring is complete. If there is, I sure don't see where -- but I'm a bit puzzled by the whole situation.

[anthonyhashemi]

@demiankatz My understanding is that it is part of jsviews, which is imported with <script src="https://cdnjs.cloudflare.com/ajax/libs/jsviews/0.9.75/jsviews.min.js"></script> in examples/index.htnmml. You are right though it isn't part of the package.json. I see universal viewer does have it in its package.json so maybe it's just poorly documented in this repo but it does seem necessary; it is referenced in comments in the interfaces in src/globals.d.ts.

@anthonyhashemi, one thing that has me a little confused: where is JSRender even coming from in this project? I don't see it in the package.json or package-lock.json. Just wondering if there's some dependency we should remove now that this refactoring is complete. If there is, I sure don't see where -- but I'm a bit puzzled by the whole situation.

[anthonyhashemi]

Thanks, @anthonyhashemi -- it looks like the build failure here is related to @edsilv's Netlify deployment. I'm not sure why this is broken or if it is still needed. I will talk to him about it and see if I can get a green checkmark, but we should be able to merge this even if the build is technically failing.

In any case, I'll also try to find time to review and test this before too long!

Thanks @demiankatz!

[demiankatz]

@demiankatz My understanding is that it is part of jsviews, which is imported with <script src="https://cdnjs.cloudflare.com/ajax/libs/jsviews/0.9.75/jsviews.min.js"></script> in examples/index.htnmml. You are right though it isn't part of the package.json. I see universal viewer does have it in its package.json so maybe it's just poorly documented in this repo but it does seem necessary; it is referenced in comments in the interfaces in src/globals.d.ts.

Ahh, interesting! I wonder if we can now remove these references, since we have eliminated the dependency on the library. I also wonder if the jsviews component is used elsewhere in the upstream UV project, or if this was the only reason for it. I imagine if it's being used anywhere else, there may be further CSP issues to worry about.

[anthonyhashemi]

@demiankatz My understanding is that it is part of jsviews, which is imported with <script src="https://cdnjs.cloudflare.com/ajax/libs/jsviews/0.9.75/jsviews.min.js"></script> in examples/index.htnmml. You are right though it isn't part of the package.json. I see universal viewer does have it in its package.json so maybe it's just poorly documented in this repo but it does seem necessary; it is referenced in comments in the interfaces in src/globals.d.ts.

Ahh, interesting! I wonder if we can now remove these references, since we have eliminated the dependency on the library. I also wonder if the jsviews component is used elsewhere in the upstream UV project, or if this was the only reason for it. I imagine if it's being used anywhere else, there may be further CSP issues to worry about.

We still need jsviews as we are still using jsobservable which is part of it https://www.jsviews.com/#jsobservable

We could maybe do work to remove that but I would prefer that be done in a separate PR to keep this PR focussed.

In terms of UV, from what I could see from my test, there was no longer a CSP issue when viewing images or pdfs. I can't be certain for other file types.

It seems to define a similar JSQuery interface as this repo referencing jsviews. It uses .view and .observable like this still does (note: I had replaced usage of .view thinking it was problematic but it is only the jsviews api https://www.jsviews.com/#jsviews which causes the issue so I brought it back)

[demiankatz]

We still need jsviews as we are still using jsobservable which is part of it https://www.jsviews.com/#jsobservable

We could maybe do work to remove that but I would prefer that be done in a separate PR to keep this PR focussed.

Agreed, if we need it for more than just the templating, we shouldn't introduce scope creep to this PR!

[demiankatz]

Thanks, @anthonyhashemi, this looks good to me! See below for a couple of questions about escaping. Apologies if either or both are "false positives," but I want to be sure we don't introduce any subtle bugs.

[demiankatz]

Thanks, @anthonyhashemi! See below for a few minor type and optimization suggestions; apologies if any are incorrect or misguided. I haven't had a chance to do hands-on testing yet, but I'll do that after you've had a chance to address this review (and when I return from my conference).

[anthonyhashemi]

Great suggestions @demiankatz . I have applied them (the types with a slight tweak) and it is still working properly.

[demiankatz]

Thanks again, @anthonyhashemi. This looks good to me and appears to be working on my end as well.

I'll leave this open for a few more days in case others want to share feedback. If I don't hear anything, I'll make the release and open a PR to integrate this new version of the gallery into the UV, so we can make sure it behaves as expected.

[anthonyhashemi]

Thanks again, @anthonyhashemi. This looks good to me and appears to be working on my end as well.

I'll leave this open for a few more days in case others want to share feedback. If I don't hear anything, I'll make the release and open a PR to integrate this new version of the gallery into the UV, so we can make sure it behaves as expected.

Brilliant, thanks, looking forward to it. Let me know if I can help with anything re release / upstream integration

[demiankatz]

As promised, this is now merged and released; here's the PR to integrate into UV: UniversalViewer/universalviewer#1406

[anthonyhashemi]

Amazing @demiankatz , will take a look at that PR