Website-viewer

[Sebastian-ubs]

new pr as follow up of #13

This is the ground work for a generic, config driven online resource viewer extensions.

This includes

• Showing how to add commands that spawn web pages inside of Platform
• example web pages
• reloading pages on scripture reference and scroll group changes, so that web pages stay up to date with the current scripture reference
• retaining the web page on layout changes and restart of platform

👉 Demo

---

This change is 

[Sebastian-ubs]

interestingly github shows commits in order of their creation date, but the order I applied them on this branch is actually

1. creating the extension with npm run create-extension -- website-viewer
2. merging the relevant previously done work from web-viewer branch into this
3. small fixes on top

[Sebastian-ubs]

as mentioned in #13

Interesting files to review are

• main.ts
• webViewerOptions.ts

Maybe

• types/online-resource-viewer.d.ts
• contributions/menus.json
• contributions/localizedStrings.json

[tjcouch-sil]

Thanks for doing this! Thanks also for the rework to use create-extension. That's really helpful, and I appreciate your time. Just wanted to publish what I have reviewed so far since I need to shift gears. Will come back to this at some point

Reviewed 35 of 41 files at r1, 2 of 4 files at r2, all commit messages.
Reviewable status: 37 of 41 files reviewed, all discussions resolved

[Sebastian-ubs]

How to add a new resource: example 58aa039

[tjcouch-sil]

Awesome!! Thanks for the great work :) I have a few blocking comments, a non-blocking comment or two (you can resolve if you don't want to take action), and many info comments (no action needed unless you want to do something). Looking forward to having this in!

Reviewed 2 of 41 files at r1, 2 of 4 files at r2.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @Sebastian-ubs)

---

src/website-viewer/src/types/website-viewer.d.ts line 6 at r2 (raw file):

  export interface CommandHandlers {
    /**
     * Opens a new Website Viewer web view with a code sandbox mockup and returns the web view id

These commands don't open a new web view if there is already an existing one at that url, right? Probably worth mentioning here

---

src/website-viewer/src/types/website-viewer.d.ts line 77 at r2 (raw file):

     * @returns
     */
    'dummy.dummy': () => Promise<string | undefined>;

If you are able to get rid of the SATISFY_TS_KEY thing like I suggested, you should hopefully be able to remove this as well

---

src/website-viewer/src/main.ts line 136 at r2 (raw file):

  scrollGroupRef: ScrollGroupScrRef | undefined,
): Promise<ScriptureReference> {
  if (scrollGroupRef === undefined) return papi.scrollGroups.getScrRef();

FYI passing undefined is the same as passing nothing for a parameter, so you could combine this line and the next into one :)

(please note there are many comments that are just informational comments. Don't feel any pressure to take action on them unless you want to)

---

src/website-viewer/src/main.ts line 191 at r2 (raw file):

  if (typeof oldRef === 'number' && typeof newRef === 'number') return oldRef !== newRef;
  // both no scroll group, need to detect object difference
  if (isScrRef(oldRef) && isScrRef(newRef)) return !deepEqual(oldRef, newRef);

FYI we have a platform-scripture-utils function we usually use for comparing two scrRefs called compareScrRefs. If it returns other than 0, they're not equal :)

---

src/website-viewer/src/main.ts line 199 at r2 (raw file):

async function getUserLanguageCode(): Promise<string> {
  // TODO: implement
  return 'NOT_YET_IMPLEMENTED';

FYI if you're looking for the interface languages, you can run papi.settings.get('platform.interfaceLanguage'). Note it is a string array, not just a string

---

src/website-viewer/src/main.ts line 212 at r2 (raw file):

  // When the scripture reference changes, re-render the last webview of type "websiteViewerWebViewType"
  // This is not fired in case of "no scroll group", this is handled inside the scroll group change code
  papi.scrollGroups.onDidUpdateScrRef((scrollGroupUpdateInfo: ScrollGroupUpdateInfo) => {

This and all these following events return unsubscribers that need to be run to deactivate your extension. Please add them to context.registrations :)

---

src/website-viewer/src/main.ts line 240 at r2 (raw file):

    if (!commandByWebViewId.has(webViewId)) return;

    const command = commandByWebViewId.get(webViewId) || SATISFY_TS_KEY;

I think you shouldn't need to use something like SATISFY_TS_KEY. Here, you can instead just return if command is undefined after this line.

---

src/website-viewer/src/main.ts line 283 at r2 (raw file):

  context.registrations.add(await websiteViewerWebViewProviderPromise);
  Promise.all(commandPromises)
    .then((arr) => context.registrations.add(...arr))

Though it is nice and convenient to use the old promise chain syntax sometimes, the code style guide says to avoid using it where possible unless there is a compelling reason. Please change this to use await and try/catch or add a compelling reason in a code comment

---

src/website-viewer/src/websiteViewerOptions.ts line 14 at r2 (raw file):

// satisfy typescript, although we do not expect these to appear
export const SATISFY_TS_KEY: keyof CommandHandlers = 'dummy.dummy';
export const SATISFY_TS_OPTIONS: WebsiteViewerOptions = {

This name is a bit confusing. This object is generally used as a sort of "default" object for WebsiteViewerOptions when there isn't another one, right? Maybe consider renaming DEFAULT_WEBSITE_VIEWER_OPTIONS?

---

src/website-viewer/src/websiteViewerOptions.ts line 24 at r2 (raw file):

export enum RefChange {
  DO_NOT_WATCH,

We have traditionally made enumeration members PascalCase :) but I don't think we have explicit style rules about it, so do what you want with this information

---

src/website-viewer/src/websiteViewerOptions.ts line 30 at r2 (raw file):

}

function range(start: number, end: number) {

Code style guide says to use verbs at the start of method names to make it clearer to understand what the variable is (I'd suggest maybe something like createRange or createArrayOfIntegersBetween). Additionally, I find this function hard to understand without reading the code because its name is not particularly descriptive. When that is the case, we usually try to make the method name more descriptive or put TSDocs on the method like this:

/**
 * Some description
 *
 * @param start description
...
 */

Note VS Code will make this for you and include the parameters for you if you type /** and press enter above the method

---

src/website-viewer/src/websiteViewerOptions.ts line 36 at r2 (raw file):

}

function englishBookNameMarble(scrRef: ScriptureReference) {

This and the following function also need verbs at the start. Maybe just add get at the start?

---

src/website-viewer/src/websiteViewerOptions.ts line 138 at r2 (raw file):

  };

  return new Map<keyof CommandHandlers, WebsiteViewerOptions>([

If you make this Map<keyof CommandHandlers | undefined, WebsiteViewerOptions>, it might make it so you don't have to use SATISFY_TS_KEY for this map

[Sebastian-ubs]

Reviewable status: 36 of 42 files reviewed, 4 unresolved discussions (waiting on @tjcouch-sil)

---

src/website-viewer/src/main.ts line 136 at r2 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

FYI passing undefined is the same as passing nothing for a parameter, so you could combine this line and the next into one :)

(please note there are many comments that are just informational comments. Don't feel any pressure to take action on them unless you want to)

done, thanks

---

src/website-viewer/src/main.ts line 191 at r2 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

FYI we have a platform-scripture-utils function we usually use for comparing two scrRefs called compareScrRefs. If it returns other than 0, they're not equal :)

done - although I am wondering about the real difference. comparing the two objects seems to work similarly good.

---

src/website-viewer/src/main.ts line 199 at r2 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

FYI if you're looking for the interface languages, you can run papi.settings.get('platform.interfaceLanguage'). Note it is a string array, not just a string

❤️

---

src/website-viewer/src/main.ts line 212 at r2 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

This and all these following events return unsubscribers that need to be run to deactivate your extension. Please add them to context.registrations :)

done

---

src/website-viewer/src/main.ts line 240 at r2 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

I think you shouldn't need to use something like SATISFY_TS_KEY. Here, you can instead just return if command is undefined after this line.

done

---

src/website-viewer/src/main.ts line 283 at r2 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Though it is nice and convenient to use the old promise chain syntax sometimes, the code style guide says to avoid using it where possible unless there is a compelling reason. Please change this to use await and try/catch or add a compelling reason in a code comment

I've changed it, please have a look if this is better now

---

src/website-viewer/src/websiteViewerOptions.ts line 14 at r2 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

This name is a bit confusing. This object is generally used as a sort of "default" object for WebsiteViewerOptions when there isn't another one, right? Maybe consider renaming DEFAULT_WEBSITE_VIEWER_OPTIONS?

done

---

src/website-viewer/src/websiteViewerOptions.ts line 24 at r2 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

We have traditionally made enumeration members PascalCase :) but I don't think we have explicit style rules about it, so do what you want with this information

done

---

src/website-viewer/src/websiteViewerOptions.ts line 30 at r2 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Code style guide says to use verbs at the start of method names to make it clearer to understand what the variable is (I'd suggest maybe something like createRange or createArrayOfIntegersBetween). Additionally, I find this function hard to understand without reading the code because its name is not particularly descriptive. When that is the case, we usually try to make the method name more descriptive or put TSDocs on the method like this:

/**
 * Some description
 *
 * @param start description
...
 */

Note VS Code will make this for you and include the parameters for you if you type /** and press enter above the method

done

---

src/website-viewer/src/websiteViewerOptions.ts line 36 at r2 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

This and the following function also need verbs at the start. Maybe just add get at the start?

done

---

src/website-viewer/src/websiteViewerOptions.ts line 138 at r2 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

If you make this Map<keyof CommandHandlers | undefined, WebsiteViewerOptions>, it might make it so you don't have to use SATISFY_TS_KEY for this map

decided to return in the get calls instead, otherwise I have to do this handling for registering the command, which seems odd

---

src/website-viewer/src/types/website-viewer.d.ts line 6 at r2 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

These commands don't open a new web view if there is already an existing one at that url, right? Probably worth mentioning here

done

---

src/website-viewer/src/types/website-viewer.d.ts line 77 at r2 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

If you are able to get rid of the SATISFY_TS_KEY thing like I suggested, you should hopefully be able to remove this as well

done, I was able to remove it, thanks.

[Sebastian-ubs]

Thank you TJ. I hope I applied all review comments.
The remaining issue I see is with imports, where I am not sure how to resolve them.

Unable to resolve path to module 'papi-shared-types'.

'shared/services/scroll-group.service-model' import is restricted from being used by a pattern. Importing from this path is not allowed. Try importing from @papi/core. Imports from paths like 'shared', 'renderer', 'node', 'client' and 'main' are not allowed to prevent unnecessary import break.

'shared/services/web-view.service-model' import is restricted from being used by a pattern. Importing from this path is not allowed. Try importing from @papi/core. Imports from paths like 'shared', 'renderer', 'node', 'client' and 'main' are not allowed to prevent unnecessary import break.

Reviewable status: 36 of 42 files reviewed, 4 unresolved discussions (waiting on @tjcouch-sil)

[tjcouch-sil]

Thanks for your time and effort on the adjustments! It's very close :) just one more thing.

Reviewed 2 of 6 files at r3, 2 of 3 files at r4, 2 of 2 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Sebastian-ubs)

---

src/website-viewer/src/main.ts line 191 at r2 (raw file):

Previously, Sebastian-ubs wrote…

done - although I am wondering about the real difference. comparing the two objects seems to work similarly good.

Thanks for the update. Using an existing function makes it easier to change how that function works everywhere all at once in the future. :) For example, if we one day need to do some slightly different comparison with it or something, we can do that. Like if we one day take versification into account.

---

src/website-viewer/src/main.ts line 283 at r2 (raw file):

Previously, Sebastian-ubs wrote…

I've changed it, please have a look if this is better now

Almost got it! Sorry, promises in TS are rather tricky. Doing a forEach on commandPromises will make it so the extension will finish activating before those promises resolve and add the unsubscribers to the registration inside the forEach because forEach doesn't work asynchronously; it all runs synchronously, so all the async function calls you're making inside it are not getting awaited anywhere. To properly wait on all the commandPromises, you can use Promise.all similarly to how you were doing it before. I think you should be able to do something like this:

context.registrations.add(
  ...(await Promise.all(commandPromises)),
  await websiteViewerWebViewProviderPromise,
  ...
);

This awaits all the commandPromises, then destructures the returned array of unsubscribers to be added to context.registrations :)

---

src/website-viewer/src/types/website-viewer.d.ts line 77 at r2 (raw file):

Previously, Sebastian-ubs wrote…

done, I was able to remove it, thanks.

Thank you for your effort on it! :)

[tjcouch-sil]

Nice! Thanks again :) oh and thanks. And thank you! Oh and I just want to make sure I remembered to say thank you! Thank you!

Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Sebastian-ubs)