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

Finish shimming require & others in extension host #213

Merged
merged 1 commit into from
Jun 1, 2023

Conversation

irahopkinson
Copy link
Contributor

@irahopkinson irahopkinson commented Jun 1, 2023


This change is Reviewable

Copy link
Member

@tjcouch-sil tjcouch-sil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks awesome! So excited to have this out of the way. It has been nagging my brain for months now. Glad it was this easy. Just one request.

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @irahopkinson)


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

  // Replace fetch with papi.fetch.
  // eslint-disable-next-line no-global-assign
  globalThis.fetch = papi.fetch;

Thanks for making this change! Cool :)

Could you please also make this change for web views so the frontend can also now use fetch appropriately?

Probably would just be replacing this line with window.fetch = papi.fetch;

I guess filing an issue would also be ok.

- try imports in 'evil' extension
- don't need to put require and others back since the extension host is in its own process
- replace `fetch` with `papi.fetch`
- also remove `.promise` after #202
Copy link
Contributor Author

@irahopkinson irahopkinson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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

Previously, tjcouch-sil (TJ Couch) wrote…

Thanks for making this change! Cool :)

Could you please also make this change for web views so the frontend can also now use fetch appropriately?

Probably would just be replacing this line with window.fetch = papi.fetch;

I guess filing an issue would also be ok.

Done. Thanks for remembering that needed changing too.

Copy link
Member

@tjcouch-sil tjcouch-sil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm: Thanks for the hard work on this!

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


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

Previously, irahopkinson (Ira Hopkinson) wrote…

Done. Thanks for remembering that needed changing too.

No prob! If you wouldn't mind, could you please temporarily change hello-world.web-view.tsx to use fetch instead of papi.fetch, put a breakpoint in internet.service.ts on line 10 inside the papiFetch in DevTools in the renderer, and see if it hits just to be absolutely sure the shim worked? I think it did; just want a sanity check to be certain.

Copy link
Contributor Author

@irahopkinson irahopkinson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @irahopkinson)


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

Previously, tjcouch-sil (TJ Couch) wrote…

No prob! If you wouldn't mind, could you please temporarily change hello-world.web-view.tsx to use fetch instead of papi.fetch, put a breakpoint in internet.service.ts on line 10 inside the papiFetch in DevTools in the renderer, and see if it hits just to be absolutely sure the shim worked? I think it did; just want a sanity check to be certain.

Yep, I tried that and it stopped on the breakpoint. I checked the compiled JS and despite the source calling fetch the compile code still has papi.fetch, because of the assignment of window.fetch = papi.fetch;, which in hindsight should be expected.

@irahopkinson irahopkinson merged commit 02dc800 into main Jun 1, 2023
@irahopkinson irahopkinson deleted the 75-shim-require branch June 1, 2023 22:09
Copy link
Member

@tjcouch-sil tjcouch-sil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved


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

Previously, irahopkinson (Ira Hopkinson) wrote…

Yep, I tried that and it stopped on the breakpoint. I checked the compiled JS and despite the source calling fetch the compile code still has papi.fetch, because of the assignment of window.fetch = papi.fetch;, which in hindsight should be expected.

Hmm actually I would expect the built hello-world.web-view.tsx not to have papi.fetch in it if you removed it because the web view doesn't know that code got shimmed in. In the web view's perspective, it's just calling fetch. Do you know if it didn't build for some reason or something...?

lyonsil pushed a commit that referenced this pull request Jun 13, 2023
- try imports in 'evil' extension
- don't need to put require and others back since the extension host is in its own process
- replace `fetch` with `papi.fetch`
- also remove `.promise` after #202
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.

Complete shimming out require and others in extension host
2 participants