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(electron): add platform support #549

Open
wants to merge 36 commits into
base: master
Choose a base branch
from

Conversation

pjoriginal
Copy link

@pjoriginal pjoriginal commented Dec 13, 2022

Platforms affected

Electron

Motivation and Context

This is required so that cordova electron can have a working file plugin

fixes #334
fixes #312
fixes #477
closes #371
closes #387

Description

The base implementation has two things.
1. Scripts to add and remove node implementations done in files cdv-electron-main.js and cdv-electron-preload.js. Since the scripts look for comments, and remove code between the comments on plugin remove, we should be good as long as the dev doesn't mess those up. These scripts are added to support Context Isloation that has been encouraged in electron@^3.0. The fs required can be called from window.cordovaFilePlugin.function. I had to tweak some changes because the return value objects have their methods removed.

With the help of Erisu, I was able to update the code so that it doesn't require extra scripts. The electron implementation uses the framework keyword and uses backend node-js functions to implement stuff.

Testing

I've tested them on my personal projects. Will write an exclusive test project if necessary,
Also, https://github.com/pjoriginal/cordova-file-test

Checklist

  • I've run the tests to see all new and existing tests pass
  • Commit is prefixed with (platform) if this change only applies to one platform (e.g. (android))
  • If this Pull Request resolves an issue, I linked to the issue in the text above (and used the correct keyword to close issues using keywords)
  • I've updated the documentation if necessary

@erisu erisu changed the title (electron) Added cordova file functionality to electron feat(electron): added platform support Dec 23, 2022
@erisu erisu changed the title feat(electron): added platform support feat(electron): add platform support Dec 23, 2022
@breautek
Copy link
Contributor

breautek commented Jan 5, 2023

The Android & iOS tests can be ignored because there are existing issues present that is causing them to fail, and that's been happening to some time. We know it's not caused by this PR.

Lint test should pass, which it currently does.

Will write an exclusive test project if necessary

Obviously it will be nice to have actual unit tests, but I won't personally reject a PR without unit tests. It can always be added later.

@pjoriginal pjoriginal closed this Jan 10, 2023
@pjoriginal pjoriginal reopened this Jan 10, 2023
@pjoriginal
Copy link
Author

I'm sorry.
I accidentally clicked on "close this issue" while trying to comment on the issue.

The test project has been created. Will add unit tests soon

@pjoriginal
Copy link
Author

@breautek added functionality and added electron stuff to tests where missing. Also fixed eslint.

@breautek breautek added this to the 8.x milestone Apr 28, 2023
@regnete
Copy link

regnete commented Aug 23, 2023

When will this be committed?

@breautek breautek modified the milestones: 8.x, 9.0.0 Dec 13, 2023
@pjoriginal
Copy link
Author

Making chnages to make this compatible to electron@nightly

Updated files to accept new single array args
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants