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

Update foam cli #3

Merged
merged 6 commits into from
Mar 29, 2022

Conversation

3mp3ri0r
Copy link
Contributor

@3mp3ri0r 3mp3ri0r commented Mar 26, 2022

Update foam-cli to work on latest core as discussed in #2.

Needs to do:

  • Update package version
  • Copy core from foam version 0.17.6
  • Update foam-cli internal dependencies to core and package.json and test foam-cli
  • Update github actions workflow on PR (run lint and test)
  • Update github actions workflow on release (publish to npm)

@3mp3ri0r 3mp3ri0r marked this pull request as ready for review March 26, 2022 16:36
@3mp3ri0r
Copy link
Contributor Author

Hi @riccardoferretti. Please check this PR. I've tried it and run perfectly using the latest core from foam v0.17.6.

Also please add NPM_TOKEN to the dependabot secret to make github actions authorized to push package to npmjs.

Copy link
Contributor

@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 updating foam-cli to the laster foam-core.

I also like the publish workflow, interesting even for Foam itself (with a few changes). I will merge it and tune it to our needs (e.g. be based on tagging).

Out of curiosity, have you considered a "repeatable" pattern to update the foam-core directory?
One thing I thought of in the past was to release just the contents of the foam-core directory (in the main Foam repo) as its own npm module, which could then be consumed by the likes of foam-cli or other clients. I haven't had time to investigate that yet though.

@3mp3ri0r 3mp3ri0r changed the title [WIP] Update foam cli Update foam cli Mar 27, 2022
@3mp3ri0r
Copy link
Contributor Author

Out of curiosity, have you considered a "repeatable" pattern to update the foam-core directory?\nOne thing I thought of in the past was to release just the contents of the foam-core directory (in the main Foam repo) as its own npm module, which could then be consumed by the likes of foam-cli or other clients. I haven't had time to investigate that yet though.

I saw until v0.11.0 Foam the develop this way, right? You want me to try break foam into modules like the old one (foam-core, foam-vscode, and foam-cli), @riccardoferretti?

@riccardoferretti
Copy link
Contributor

That's the way it was developed, but it was very cumbersome DX wise (basically working on the extension was much more complicated because of that setup, both from a dev and a build POV), which is the reason why I merged foam-core into foam-vscode. And I was aware that the price to pay was the publishing of the foam-core package and its use in foam-cli (and other third party clients).

I wouldn't want to go back to individual packages in Foam, but as you might have noticed the core directory inside foam-vscode is basically its own package (no dependencies outside of the dir, which is a design choice that is made on purpose), so I was considering creative solutions to publish just that directory as its own module.
I know it's an unorthodox approach to the problem, but this way the benefits of the current directory setup would not be lost while still be able to publish foam-core, which would allow to update foam-cli more easily in the future.

Makes sense?

@3mp3ri0r
Copy link
Contributor Author

so I was considering creative solutions to publish just that directory as its own module.

I got your pain point but I'm not sure about the solution. Which one did you mean:

  1. Move the core from foam-vscode to its own repo and foam-vscode and foam-cli used the core as submodule
  2. Move the core from foam-vscode to be its own package like the old one, but without publishing it to npm, so foam-vscode and foam-cli using relative path accessing the core

Anyway, the old one you use yarn workspace to maintain monorepo for 3 modules. Have you tried modern solution like nx or turborepo, @riccardoferretti?

@riccardoferretti
Copy link
Contributor

I am not familiar with nx and turborepo, will take a look

@riccardoferretti
Copy link
Contributor

They look very interesting, but I am absolutely not familiar with them, so I am hesitant to introduce them in Foam.
In any case I will keep them on my radar, thanks for the pointers!

@riccardoferretti riccardoferretti merged commit 53af12e into foambubble:main Mar 29, 2022
@3mp3ri0r
Copy link
Contributor Author

3mp3ri0r commented Apr 3, 2022

You're welcome @riccardoferretti. How can I help you and foam team to make the foam-core modular but still easy to maintain with the other part? Some options that I recommend:

  1. Move the foam-core to it's own repository and publish it to npm package. It make the foam-core development independent and can be extensible to another use case, like foam-cli and might be the other cases. Basically it has its own roadmap.
  2. Moving out core from foam-vscode to be equivalent with foam-vscode and move in foam-cli to be equivalent with them. Will maintain the development cycle using monorepo tools like nx. The cons is roadmap will be mixed and might be tight coupling between them.

If you have any other suggestion or reference just tell me so I can help to achieve the modularity.

And anyway, when will you tag the latest main branch so it can be published to npm package?

@riccardoferretti
Copy link
Contributor

For now the one requirement I have is that DX in foam-vscode is not disrupted, so I fear both approaches would fall short of that.

Yup, I will tag the branch today!

@3mp3ri0r
Copy link
Contributor Author

3mp3ri0r commented Apr 7, 2022

For now the one requirement I have is that DX in foam-vscode is not disrupted, so I fear both approaches would fall short of that.

Okay, I'll try to see how your team works and try to create PoC on my own repo to make the DX same with current condition or even better, and We'll discuss again after.

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