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

Migrate away from vorpal #671

Open
mihkeleidast opened this issue Oct 20, 2020 · 20 comments
Open

Migrate away from vorpal #671

mihkeleidast opened this issue Oct 20, 2020 · 20 comments
Assignees
Labels
[area] cli feature-request in progress This issue is being actively looked into

Comments

@mihkeleidast
Copy link
Member

mihkeleidast commented Oct 20, 2020

What problem would this feature solve?

Vorpal has not been updated in four years, its dependencies are out of date and give our users vulnerability notices when installing Fractal.

This should also fix a deprecation warning users get when installing Fractal:

npm WARN deprecated [email protected]: core-js@<3 is no longer maintained and not recommended for usage due to the number of issues. Please, upgrade your dependencies to the actual version of core-js@3.

└─┬ @frctl/[email protected]
  └─┬ [email protected]
    └─┬ [email protected]
      ├─┬ [email protected]
      │ └── [email protected]  deduped
      └── [email protected] 

What the feature should look like?

Fractal's CLI module should be updated to use something other than vorpal.

@LeBenLeBen
Copy link
Member

What you think would be a great replacement? Inquirer.js?

@mihkeleidast
Copy link
Member Author

mihkeleidast commented Oct 25, 2020

By the looks of it, inquirer is not meant for building CLIs. Commander.js or yargs should be sufficient for the CLI purposes, while using inquirer for the interactive stuff when needed. I don't think neither commander or yargs provide real interactivity on the CLI...

Since vorpal is based on commander+inquirer, it would make sense (less code changes, i presume) to use those.

@mihkeleidast mihkeleidast self-assigned this Nov 3, 2020
@mihkeleidast
Copy link
Member Author

I'm currently looking at https://github.com/hongaar/bandersnatch as a replacement.

@Chapabu
Copy link
Member

Chapabu commented Nov 3, 2020

Hey all! I've got my CLI branch still kicking around, would you like me to resurrect it and carry on with this one? I also have a bunch of TypeScript stuff in there, but that's likely out of date now with the release of 4.x, but should be simple to get it updated.

I started moving it along with Commander purely because it's the most popular. Bandersnatch looks really cool, but 60 weekly downloads vs 50.5m worries me a little.

@mihkeleidast
Copy link
Member Author

Commander is sure more popular but I think it lacks the interactive CLI part that we currently have from Vorpal. Bandersnatch is based on yargs/enquirer (vs our current commander/inquirer dependency from Vorpal) and is "inspired by" Vorpal which by the looks of it should make it easier to migrate. Also Bandersnatch is written in TypeScript which should help with our eventual move.

I'd be happy to leave this issue to you, if it'll get solved in the near future :) Would be good to get some TS setup in with it, if you already got it.

@Chapabu
Copy link
Member

Chapabu commented Nov 4, 2020

@mihkeleidast Yeah - leave this one with me :) I'll review/update the TS stuff in here as well whilst I'm at it.

@Chapabu Chapabu self-assigned this Nov 4, 2020
@Chapabu
Copy link
Member

Chapabu commented Nov 4, 2020

@mihkeleidast Did we land on anything with the new command? Are we keeping it, or are we moving to an initialiser or create-fractal-app style approach? Or both?

@mihkeleidast
Copy link
Member Author

@mihkeleidast Did we land on anything with the new command? Are we keeping it, or are we moving to an initialiser or create-fractal-app style approach? Or both?

I'd keep it for now, we can't remove it without introducing a breaking change. But we can refactor it into something that is easily separable in the future.

@mihkeleidast mihkeleidast removed their assignment Nov 4, 2020
@joyheron
Copy link

joyheron commented Apr 6, 2021

We also are using fractal in multiple projects and have been (unhappily) ignoring the log messages about the lodash vulnerability. Is there anything I could do to help create a fix for this?

@mihkeleidast
Copy link
Member Author

Since vorpal is totally unmaintained the only solution here is to replace it completely, which is not a trivial feat :)

@Chapabu have you made any progress here? Is there something others can help with or could I maybe work on this myself?

@Chapabu
Copy link
Member

Chapabu commented Apr 6, 2021

@mihkeleidast I'll pick this one back up - I think I've got a local branch with a fair amount of the work in it, and it'd be good to get back into the weeds.

@Chapabu
Copy link
Member

Chapabu commented Apr 10, 2021

Just some progress on this. I've got the info and build commands working as part of the monorepo examples. Working on the serve command and the new command this week - although I reckon the fractal new command might be the tricky one. After that, it's just tests - so probably a week or two then this one can be checked off.

@mihkeleidast
Copy link
Member Author

@Chapabu A little ping on this again - when do you suppose this will be ready for some initial review and testing? It's becoming quite important since we decided to postpone the major release w/o Node 10 support until this is done, so it's blocking a bunch of updates.

@joyheron
Copy link

@Chapabu Is there any way that I could help with developing this feature? Would love to review/test etc. any implementations.

@dasginganinja
Copy link

@Chapabu Pinging for an update on this for testing the functionality. Looking to get these security issues cleared up.

@mihkeleidast
Copy link
Member Author

Just an update here: we last chatted about this in december to figure out what we should do with the current fractal new global command in the new CLI. As it's the only reason ever someone would need to install Fractal globally, it would complicate things a bit et cetera.

An idea was to move it to a separate initializer package, and leave the CLI only for local project installs. As the separation had already been filed as an issue, we moved in that direction (see #635 and #1131). Hopefully this will allow us to move forward in the near future 🤞

@joyheron
Copy link

I think the idea of extracting this into a separate initializer package is a good one. This is something that is usually only necessary the first time that someone sets up a project. For my specific use cases, I rarely use the fractal new command personally, because I prefer copying my fractal configuration from project to project and setting up the directories the way that I like them.

I'm also very willing to help with testing and reviews. We do have fractal as a dependency for some client projects, and it is difficult to explain to the customer why we have issues with npm audit, so it would be of great value to me if this could be solved. I'm willing to help in any way I can. 😄

@rediris
Copy link

rediris commented Jun 21, 2022

You can see we've been prodding dthree to update vorpal here: dthree/vorpal#343 (comment)

@Chapabu
Copy link
Member

Chapabu commented Aug 27, 2022

The biggest thing holding this one up from my end (outside of my time) was that the release process is a bit..well...wrong to say the least. I'm going to lump this one in with #776 (or an updated version of it) so that we can put out a major release with the breaking changes. This will allow me to completely ignore things like fractal new and the interactive CLI (that I personally have never used) and focus on the core features (build and serve).

fractal new can be replaced with an initialiser (see #1131) to leverage newer (and more common) npm features.

As discussed above, I see no reason not to use Commander, so I'll likely just reboot the work I started in 2020 🙈 .

@dthree
Copy link

dthree commented Oct 12, 2022

💀 Sorry guys

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[area] cli feature-request in progress This issue is being actively looked into
Projects
None yet
Development

No branches or pull requests

7 participants