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

feat(core): implement CapacitorCustomPlatform for 3rd party platforms #4771

Merged
merged 22 commits into from
Aug 12, 2021
Merged

Conversation

IT-MikeS
Copy link
Contributor

@IT-MikeS IT-MikeS commented Jun 28, 2021

The whole API for platforms may not work out in the current form but with the changes outlined in this, at least from Electron point of view, will function much better with how Context Isolation works.

See next branch of Electron platform for working example. https://github.com/capacitor-community/electron/tree/next

@thomasvidas
Copy link
Contributor

This seems fine when I pulled down this and the electron build, it is failing the lint so could you fix that to make sure it passes the rest of the tests? Also tagged @jcesarmobile as a reviewer so he could look over it too, but this seems good to me

core/src/definitions-internal.ts Outdated Show resolved Hide resolved
core/src/runtime.ts Outdated Show resolved Hide resolved
@IT-MikeS
Copy link
Contributor Author

IT-MikeS commented Jun 30, 2021

Not sure why lint test failed, it uses the same code base as the main repo. Says the issue is:

558:23  warning  '_pluginId' is defined but never used  @typescript-eslint/no-unused-vars
558:34  warning  '_fn' is defined but never used        @typescript-eslint/no-unused-vars

However the use of a _ should be suppressing that warning.

@jcesarmobile
Copy link
Member

those are warnings, not errors, try deleting node_modules and running npm install again and then npm run fmt from the root of the repository

@IT-MikeS
Copy link
Contributor Author

Now I have 151 changes to commit 😅

@jcesarmobile
Copy link
Member

then don't commit, I'll take a look

@IT-MikeS
Copy link
Contributor Author

Yeah figured that'd be a bad commit

@IT-MikeS IT-MikeS marked this pull request as ready for review June 30, 2021 17:09
@IT-MikeS IT-MikeS changed the title Custom platform support change fix(core): custom platform support changes to support context isolation injected plugins Jun 30, 2021
@IT-MikeS
Copy link
Contributor Author

What was the lint issue?

@jcesarmobile
Copy link
Member

I just ran npm run fmt, not sure why your computer changed more things

@IT-MikeS
Copy link
Contributor Author

IT-MikeS commented Jul 1, 2021

It seems to have been it wasn't using ignore lists probably and formatting a bunch of files it shouldn't have been

@IT-MikeS
Copy link
Contributor Author

IT-MikeS commented Jul 1, 2021

Pushed out a v4.0.0-alpha.7 of the electron platform tested with my device and filesystem plugin example in the repo's plugin-examples folder, all went well with these changes in place 👍

Copy link
Contributor

@thomasvidas thomasvidas left a comment

Choose a reason for hiding this comment

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

Been messing with this over the weekend and didn't run into any major issues, so approving this 👍 @jcesarmobile

Copy link
Member

@jcesarmobile jcesarmobile left a comment

Choose a reason for hiding this comment

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

Do you think this can be accomplished in a non breaking way? deprecating the platforms API instead of removing it?
We are not sure at this point if other custom platforms exist, there has been a few people asking about how to create custom platforms.

Also, would be good if you could give a better insight on why this is needed for supporting context isolation, all I see in this change is platforms plugins defaulting to web plugins if not defined in native, but don't really see how this would fix the native plugins.

@IT-MikeS
Copy link
Contributor Author

IT-MikeS commented Jul 6, 2021

Do you think this can be accomplished in a non breaking way? deprecating the platforms API instead of removing it?
We are not sure at this point if other custom platforms exist, there has been a few people asking about how to create custom platforms.

Also, would be good if you could give a better insight on why this is needed for supporting context isolation, all I see in this change is platforms plugins defaulting to web plugins if not defined in native, but don't really see how this would fix the native plugins.

We can deprecate sure. I don't know if any other custom platforms exist beyond my own tinkers with the likes of uwp, tauri, etc., however this should be easy to use with others in this form and this seems like an easier way to do custom platform plugins overall.

The main reason we have to do something like these changes is that the platforms API as it is, assumes that the platform has access to the app in a way where it can initialize the capacitor runtime prior to the web app being loaded.

However in the case of the context isolation in electron you can not initialize the runtime as the window object etc. are not shared between the preload script and the web app. You can expose properties to the web app's window object however they are limited in what can be copied across the context bridge. Thus exposing the window.CapacitorCustomPlatform object containing the platform name (for use with getPlatform() etc.) and a plugins object that can be mapped by the current load plugin methods is a good way to solve this while using code that already exists in the capacitor runtime. These mapped objects relate to the way the custom platform handles plugin calls natively.

So in the electron platform's case, the platform generates a set of function calls that implement IPC dynamically on both the renderer side and the main process side. These IPC calls allow plugin code to run in the main electron process on node-js, but only if they are generated by the platform, making it safer by not allowing node integration on the renederer process which could be leveraged by remote code. (read more here https://www.electronjs.org/docs/tutorial/context-isolation)

On using web versions of plugins by default, this is more of an electron thing as far as I know, but because electron is a full chromium browser essentially, web plugins should function just fine on there, and lots of plugins may not need native electron options because of that.

@jcesarmobile
Copy link
Member

Well, even if there aren't any other platforms, it will break current electron platform until users update, so better deprecate it now and we will remove it before capacitor 4.0.0

core/src/runtime.ts Outdated Show resolved Hide resolved
@IT-MikeS
Copy link
Contributor Author

IT-MikeS commented Jul 6, 2021

Okay I will make the adjustments to this PR for deprecation then re-request review 👍

core/src/runtime.ts Outdated Show resolved Hide resolved
Co-authored-by: jcesarmobile <[email protected]>
@IT-MikeS
Copy link
Contributor Author

IT-MikeS commented Jul 7, 2021

I have added back in the old platform API and set it to deprecated

@IT-MikeS
Copy link
Contributor Author

Is there anything else you'd like to see @jcesarmobile ?

@jcesarmobile jcesarmobile changed the title fix(core): custom platform support changes to support context isolation injected plugins feat(core): implement CapacitorCustomPlatform for 3rd party platforms Aug 11, 2021
Copy link
Member

@jcesarmobile jcesarmobile left a comment

Choose a reason for hiding this comment

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

works fine and doesn't break existing platforms

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.

4 participants