Added app://cache/extension-types for extension type sharing

[tjcouch-sil]

For an explanation of why I added app://cache/extension-types instead of using app://cache/extensions, see my comment on issue 365.

Also added app://installed-extensions for an easy place to put extensions

Goes along with paranext/paranext-extension-template#52

Relates to #365

---

This change is 

[irahopkinson]

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

[lyonsil]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @tjcouch-sil)

---

src/extension-host/services/extension.service.ts line 377 at r1 (raw file):

      // Completely ignore extensions that do not have `main` at all as a hint to developers
      if (settled.value.main === undefined) {
        logger.error(

Why did you change this to error? The platform will still run even if this happens. I thought "error" was reserved for fatal errors.

[lyonsil]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @tjcouch-sil)

---

src/extension-host/services/extension.service.ts line 388 at r1 (raw file):

    .map((fulfilled) => (fulfilled as PromiseFulfilledResult<ExtensionInfo>).value);

  // Filter out duplicate extensions. Only the first extension encountered in order is used

It might be too early to think about this, but what happens once we start encountering different versions of the same extension? Does being earlier in a path trump a higher version number, or does a higher version number trump being earlier in the paths?

---

src/extension-host/services/extension.service.ts line 742 at r1 (raw file):

      await cacheExtensionTypeDefinitions(allExtensions);
    } catch (e) {
      logger.error(`Could not cache extension type definitions: ${e}`);

I guess this is similar to my comment earlier, but probably even more so since the type definitions don't impact the application at runtime. Why an error instead of a warning?

[tjcouch-sil]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @lyonsil)

---

src/extension-host/services/extension.service.ts line 377 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

Why did you change this to error? The platform will still run even if this happens. I thought "error" was reserved for fatal errors.

I made this an error because its purpose is to inform extension developers that they did something wrong. The whole extension is thrown out if there isn't a main provided. In my mind, this seems to be more fitting as an error than just a warning because it indicates the extension developer did something explicitly wrong according to our definitions that we reject, not just that they did something possibly wrong or potentially hazardous. Especially now that we are doing things with extensions other than activating them (caching their types), but these extensions that do not have a main provided are rejected and not considered for the other things we do with extensions, this seems like a more serious issue than just that the extension doesn't get activated. Something is actually not happening because of this issue, and extension devs need to know that.

Also, I believe we discussed while looking at #353 that we don't currently have clear delineations for what kinds of things go into what log levels and that developers can put what they subjectively feel is appropriate for now. As such, I don't know of any particular restrictions we have ever had on what we log as an error. If I'm understanding what you propose as fatal error to mean an error that causes something in the software to fail, it seems we log a number of things throughout our code including in this file as error that do not match that definition. I just use error when I want to show something that is a very serious problem. Or if you consider an extension not loading to be a fatal error, I would argue that this counts as a fatal error anyway.

---

src/extension-host/services/extension.service.ts line 388 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

It might be too early to think about this, but what happens once we start encountering different versions of the same extension? Does being earlier in a path trump a higher version number, or does a higher version number trump being earlier in the paths?

According to current code, earlier in the path does indeed trump a higher version number. I thought that seemed fine for the time being since we don't really have any concept of version numbers at the moment and this matches the functionality of typeRoots to some extent (hopefully makes all of this easier for people to understand). I also thought it would be weird to be developing an extension locally and for that code not to be run regardless of version numbers (I don't imagine this will happen much... Haha but I guess maybe it could happen if someone wants to make a minor version patch or something 🤔). I imagine we may have to do some significant rework in the end anyway to support multiple versions of an extension running together.

One thing I did consider was telling people their typescript modules need to have their major version attached to make this kind of situation easier to handle. But I don't know if it's really the right time to worry about that yet.

Created #661 about this

---

src/extension-host/services/extension.service.ts line 742 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

I guess this is similar to my comment earlier, but probably even more so since the type definitions don't impact the application at runtime. Why an error instead of a warning?

Seems like a serious problem if you're developing and this doesn't succeed for some reason. But I appreciate that it is less significant a problem than the other problem, so I'll demote it.

[lyonsil]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @tjcouch-sil)

---

src/extension-host/services/extension.service.ts line 377 at r1 (raw file):

Also, I believe we discussed while looking at #353 that we don't currently have clear delineations for what kinds of things go into what log levels and that developers can put what they subjectively feel is appropriate for now.

A couple of things about this:

1. I agree we didn't want to force aligning on meanings to move forward with that ticket. I figured we would have a bunch of conversations like this slowly over time that would help form what definitions make sense to us as a group, though.
2. I worry about how often we're running into cases where things get changed based on the preferences of who last touched a particular part of code. That thought isn't about log levels specifically or about you specifically. I think it happens with my PRs, too. Maybe it's all fine, but something about it feels off.

Anyway, I'm unblocking this comment to move forward. I can see failing to load an extension to be on the same level as failing to load the platform overall in terms of logging. Thanks for sharing your thoughts!

---

src/extension-host/services/extension.service.ts line 388 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

According to current code, earlier in the path does indeed trump a higher version number. I thought that seemed fine for the time being since we don't really have any concept of version numbers at the moment and this matches the functionality of typeRoots to some extent (hopefully makes all of this easier for people to understand). I also thought it would be weird to be developing an extension locally and for that code not to be run regardless of version numbers (I don't imagine this will happen much... Haha but I guess maybe it could happen if someone wants to make a minor version patch or something 🤔). I imagine we may have to do some significant rework in the end anyway to support multiple versions of an extension running together.

One thing I did consider was telling people their typescript modules need to have their major version attached to make this kind of situation easier to handle. But I don't know if it's really the right time to worry about that yet.

Created #661 about this

Sure - I was just wondering about this as I read the code and wanted to plant some seeds.

[lyonsil]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @tjcouch-sil)

---

src/extension-host/services/extension.service.ts line 742 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Seems like a serious problem if you're developing and this doesn't succeed for some reason. But I appreciate that it is less significant a problem than the other problem, so I'll demote it.

Thanks - I think it would be good to change this to a warning. I'll change this to non-blocking in case you have some other follow up change where it would be easier to make this adjustment.

[tjcouch-sil]

Reviewable status: 1 of 5 files reviewed, all discussions resolved (waiting on @irahopkinson)

---

src/extension-host/services/extension.service.ts line 377 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

Also, I believe we discussed while looking at #353 that we don't currently have clear delineations for what kinds of things go into what log levels and that developers can put what they subjectively feel is appropriate for now.

A couple of things about this:

1. I agree we didn't want to force aligning on meanings to move forward with that ticket. I figured we would have a bunch of conversations like this slowly over time that would help form what definitions make sense to us as a group, though.
2. I worry about how often we're running into cases where things get changed based on the preferences of who last touched a particular part of code. That thought isn't about log levels specifically or about you specifically. I think it happens with my PRs, too. Maybe it's all fine, but something about it feels off.

Anyway, I'm unblocking this comment to move forward. I can see failing to load an extension to be on the same level as failing to load the platform overall in terms of logging. Thanks for sharing your thoughts!

1. Agreed. I think something as significant as "error is only for fatal errors" would be good to discuss with the team to make sure everyone wants it and that we agree on a definition for "fatal error" because I think that would be a significant change from what I have understood it to mean and what I imagine others might understand as well. Seems very few things are literally fatal errors in the strict sense of causing the software to fail in the JavaScript world - take the DevTools console for example; it has numerous errors that don't cause the site to fold on practically every website ;) I guess one thing we may also want to consider is for whom we're making the log in general. Log levels impact extension developers, so maybe we should also consider what they would imagine the log levels mean since probably many of them won't read our guidelines around log levels even if we write them haha or maybe we don't need to worry about it if we just make the logs a bit more contextual - spelling out very clearly (probably preferably next to the log level for ease of "Find"ing) which extension (or core) the log comes from.
2. Agreed that it's generally not good practice to go around changing things unrelated to your PR, especially if it's just preference. Even small reworks could be hazardous without proper unit tests... When you're reworking code that is related to your PR for some real benefit like performance improvements, not just preference, is where some gray area gets in from my perspective. Would be glad to discuss and think through more implications and guidelines for such things. Anyway, in this particular case, I originally wrote the warning, and I changed it because I was already in this chunk of code, I thought I had written it as an error originally, and the consequences of this problem increased in this PR as well. Seemed worth changing from my perspective.

---

src/extension-host/services/extension.service.ts line 742 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

Thanks - I think it would be good to change this to a warning. I'll change this to non-blocking in case you have some other follow up change where it would be easier to make this adjustment.

Sorry, just took a minute because I decided in the end to add types to the manifest.json.

[lyonsil]

Reviewed 1 of 3 files at r1, 4 of 4 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved