172 split papi #247

irahopkinson · 2023-06-15T07:38:33Z

Split papi

limit the scope of the webview service available in papi.

Remove papi circular dependency

previously papi imported the webview service which imported papi.
the items that used papi in the webview service have been moved to the renderer globalThis directly.

This change is

lyonsil

This looks much cleaner than what we had previously. The workaround for the circular dependency was nice, too. Good work!

My only question at this point is about the shared papi.d.ts. Does it matter that everything is in one d.ts file? That is, when extension developers are working from the shared d.ts file, will it show them more than is available in their given context (i.e., renderer vs. extension host)?

Reviewed 10 of 10 files at r1, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @irahopkinson)

tjcouch-sil

Thanks for the hard work on this! Looking forward to having this improvement in place. I have some blocking suggestions (related to this split in the types that we're discussing here), but I will put everything as non-blocking so you can just resolve stuff when you're done if you think you have addressed it appropriately as I don't know when I will look at this next.

I believe we discussed making the frontend and the backend papis named something different to differentiate them from each other similar to what Matt mentioned (but without generating two files - I guess we could generate two separate files if we wanted, but I think this is cleaner since it doesn't generate the same type dependencies into two different files and lets us have just one npm package instead of two. We could leverage the eslint rule no-restricted-imports to enforce the right import in the right environment). Something like "papi-renderer" or "papi-frontend" or something vs "papi-extension-host" or "papi-backend" or something.

Then in the main file part, someone would write import papi from 'papi-backend' and would get the appropriate types for the main file's papi.

In webviews, someone would write import papi from 'papi-frontend' and would get the appropriate types for the renderer's papi.

Reviewed 10 of 10 files at r1, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @irahopkinson)

lib/papi-dts/edit-papi-d-ts.ts line 17 at r1 (raw file):

// Rename module 'shared/services/papi.service' to 'papi' so extensions can import just 'papi'
papiDTS = papiDTS
  .replaceAll("'renderer/services/papi-renderer.service'", "'papi'")

I believe we discussed making this named something different to differentiate it from the backend papi. Something like "papi-renderer" or "papi-frontend" or something.

lib/papi-dts/edit-papi-d-ts.ts line 18 at r1 (raw file):

papiDTS = papiDTS
  .replaceAll("'renderer/services/papi-renderer.service'", "'papi'")
  .replaceAll("'extension-host/services/papi-extension-host.service'", "'papi'");

I believe we discussed making this named something different to differentiate it from the frontend papi. Something like "papi-extension-host" or "papi-backend" or something.

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

  Module.prototype.require = ((moduleName: string) => {
    // Allow the extension to import papi
    if (moduleName === 'papi') return papi;

This would be where you would change the name to 'papi-extension-host' or 'papi-backend' to correspond with the new name for the papi backend part.

src/renderer/global-this.model.ts line 26 at r1 (raw file):

 */
function webViewRequire(module: string) {
  if (module === 'papi') return papi;

This would be where you would change the name to 'papi-frontend' or 'papi-renderer' to correspond with the new name for the papi frontend part.

src/renderer/global-this.model.ts line 59 at r1 (raw file):

// Note: these items are used in `src\shared\services\web-view.service.ts`. Putting them here breaks
// the circular dependency since `papi` uses the webview service.
// TODO: Hacking in React, createRoot, and papi onto window for now so webViews can access it. Make this TypeScript-y

I would say you accomplished this goal and can remove this TODO ;)

- previously papi imported the webview service which imported papi. - the items that used papi in the webview service have been moved to the renderer globalThis directly.

- limit the scope of the webview service available in papi.

irahopkinson

Absolutley, that is part 2, this is part 1 (sorry I could have made this clear in the PR description). I reasoned that this is complete and functional enough to merge as is since it takes us a step forward and it doesn't require a change for extensions and everything still works as before (currently papi.d.ts already exposes more than is available in each context so no worse in this PR). But issue #172 isn't complete until part 2 is sorted.

Reviewable status: 9 of 10 files reviewed, all discussions resolved (waiting on @lyonsil and @tjcouch-sil)

lib/papi-dts/edit-papi-d-ts.ts line 17 at r1 (raw file):