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

[WIP] Enable web extension for Foam #1290

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

Conversation

pderaaij
Copy link
Collaborator

I am making a fresh attempt on getting Foam to work on the browser. I'll close #1127 later on, but am keeping it for reference. So far, so good. But long way to go.

@pderaaij
Copy link
Collaborator Author

Update:

The extension is running quite well in the online environment at first sight. To be sure - and for regression, I'd like to run the integration tests for web as well. This is where the pain begins.

Last time I got stuck in a long loop to get Mocha working, so this time I went for vitest. However, this is not an easy ride as well. Vitest works well for unit tests, but integration tests are the problem. For reference, we need to run the test suite programmatically. We need to start the vscode environment and then run the test suite (see: run-tests/ts). Support for programmatically is limited in vitest (https://vitest.dev/advanced/api.html). However, this only works in ESM mode. Vscode, due to electron, runs only commonjs (microsoft/vscode#130367).

There seems to be some ways to get commonjs working in vscode, but I am wondering if we want all these changes just to get testing for web working.

For now, I feel vitest is not the way forward. However, I did learn a thing or two. I am going to have a fresh look at getting mocha working. But, fresh ideas are welcome.

Copy link
Collaborator

@riccardoferretti riccardoferretti left a comment

Choose a reason for hiding this comment

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

Thanks for giving it a second stab! I am happy to see that the code is basically ready for the transition, except for the readFileSync which I still think needs a solution (have you checked if the toString works?)

Unfortunately I am not super versed in mocha nor vitest, so can't help much on that front. The commonjs requirement is a bit of a limitation, and I agree that is probably better to play within that unless a solid solution can be found otherwise.

@@ -194,7 +193,7 @@ function contentExtractor(
parser: ResourceParser,
workspace: FoamWorkspace
): string {
let noteText = readFileSync(note.uri.toFsPath()).toString();
let noteText = vsWorkspace.fs.readFile(toVsCodeUri(note.uri)).toString();
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think vsWorkspace.fs.readFile returns a promise, this was to me the only open question as to how to transition in the code

Choose a reason for hiding this comment

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

Hi, is this still open? If yes can we get around by writing a new markdown-it plugin to handle the async operation?
Their documentation has some guidelines. Though I don't follow the wording aptly but I can give it a quick stab by forking markdownItRegex. WDYT?

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is still open, you can find all the info and continue the conversation here: #1295

@pderaaij
Copy link
Collaborator Author

pderaaij commented Oct 4, 2023

I've been working on getting the tests to work with Mocha. Although it seemed I was making progress I ran into a big problem. Our assertion library is jest's expect. A library that's not easily replaced by chai or a other browser compatible assertion library. So, my conclusion is that if we really want to have tests for both extension and browser we would need to overhaul the current testing library, or maintain two libraries. Both are not preferable.

My suggestion is to enable the brwoser extension and only add a test suite that checks if Foam can be activated in the web browser. A pattern that I have seen at more VSCode extensions. This way we at least confirm the webpack bundle is okay and works. Specific checks for working functionality in the browser would not exist. Only perhaps if it is a recurring event in which we would write a specific regression test.

@riccardoferretti what do you think?

@riccardoferretti
Copy link
Collaborator

Thanks @pderaaij for the investigation.

My suggestion is to enable the brwoser extension and only add a test suite that checks if Foam can be activated in the web browser. A pattern that I have seen at more VSCode extensions [...] Specific checks for working functionality in the browser would not exist. Only perhaps if it is a recurring event in which we would write a specific regression test.

Mmmm.. can you share a couple of VS Code extensions that use that approach?
I fear it might be a little brittle, but maybe I am missing something?
I guess testing the activation would validate that imports and packages are valid, but wouldn't cover the actual runtime.. then when an issue arises we could cover it with an explicit test (which requires a new assertion library, see comment below).
I am trying to figure out how big the window for uncaught bugs is with this approach..

Our assertion library is jest's expect. A library that's not easily replaced by chai or a other browser compatible assertion library. So, my conclusion is that if we really want to have tests for both extension and browser we would need to overhaul the current testing library, or maintain two libraries. Both are not preferable.

I am not against migrating over time to another library that would be more web friendly. Basically all new and edited tests would use the new library until we reach a point of switch over.
I found some articles online of people that have done such thing, so at least we know it's possible.

@pderaaij
Copy link
Collaborator Author

Annoyingly enough I can't find these examples anymore in my history and a quick GitHub search leaves me blank as well. I have researched so many repo's I might lost track how often I have seen this pattern. I do agree it feels quite brittle.

We could first rewire the testing framework. Or we just publish the web extension and rewrite the tests based on incoming reports. But that's not the greatest experience. In any case, it will require some work.

@riccardoferretti
Copy link
Collaborator

Ok, this is something to think about, I am still debating if it's "good enough" (it might be).
Can you get the PR in a place where all tests pass using jest? That feels like the first thing to ensure regardless.

I am open to your suggestion about using the legacy framework (jest) for the regular testing, and only have some smoke tests for the web extension.

@flancian
Copy link

flancian commented Jan 3, 2024

Ahoy there! I come with few skills but willing to contribute help to try to merge this if I can provide any. I also know several other people who are interested in getting Foam to work on VS Code web :)

What's left to be done? Could the extension be merged as-is without interfering with code quality/stability for regular (non-browser-based) VS Code users, maybe with a warning/alpha disclaimer?

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.

None yet

4 participants