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

Foam Web Extension #1395

Merged
merged 11 commits into from
Sep 17, 2024
Merged

Foam Web Extension #1395

merged 11 commits into from
Sep 17, 2024

Conversation

riccardoferretti
Copy link
Collaborator

This PR is heavily inspired and influenced by #1290 - thanks @pderaaij for spearheading and working through this complex feature.
See that PR for more context.

Compared to #1290 , this PR:

  • does not introduce webpack, mocha, and double TS settings
  • removes (for now) support for testing the web extension (basically, we are still, only, testing the node extension). I know, that's not great, but given the limited testing we were able to do anyways, it feels like a reasonable trade off for the reduced complexity
  • avoids some extra dependencies for the migration where possible (e.g. using a sha library that works in both node and browser)

This way:

  • we have a working web extension (although a bit brittle as it has no testing of its own - but then again I don't expect many changes that would impact the web extension without affecting the native one)
  • we have the code in a place where improvements on this side (including moving to webpack etc) can be done incrementally, as the feature is already there

The only thing I would have liked to add was running a basic test in the web extension (without the need for mocha) that would only test if the extension is present. Then again, this can be added as a second step.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is not strictly about this PR - should have not been here 😅

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is not strictly about this PR - should have not been here 😅

@pderaaij
Copy link
Collaborator

Looks good to me!

@riccardoferretti
Copy link
Collaborator Author

Great @pderaaij ! Again, I want to empathise that you basically did all the hard work here, I just repackaged it to avoid those extra dependencies 🙏

@riccardoferretti riccardoferretti merged commit dde11f8 into master Sep 17, 2024
3 checks passed
@riccardoferretti riccardoferretti deleted the feature/web-ext branch September 17, 2024 07:57
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.

2 participants