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

maintenance: upgrade to TypeScript 5, Oclif 4, node 18+ #587

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

paulRbr
Copy link
Member

@paulRbr paulRbr commented Nov 7, 2024

This is basically a re-write from scratch using a newly generated package from Oclif v4 due to a too big of a jump from Oclif v1 to oclif v4.

I've then copied files from our CLI one by one making sure compilation (and features) work.

There are still some work to be done: (all done 🎉)

  • preview cmd
  • diff cmd
  • deploy cmd
  • overlay cmd
  • Tests
    • unit
    • commands (done but there's something I'm missing they don't run!)
    • integration

Notes for the reviewer

It's a big PR, sorry for that, we can do a pair-review if you feel like it 🤗.

Also, there are a few things I had to adapt in the CLI to make this current upgrade:

  • I couldn't get the open node package to work (it was included in oclif's fancy-ux in v1 but isn't part of oclif anymore). This lead me to comment (and thus disable the --open flag features of the CLI) it works!
  • I could't find a way to test a “waiting” command (the bump preview --live one) because the mocha test process never ends due to the command “waiting”. I tried with mocking the stdin (as described here) but I don't think an “exit signal” character exists which I can send to stdin to stop the running command. This lead me to comment the related test.

Closes #448

@paulRbr paulRbr self-assigned this Nov 7, 2024
@paulRbr paulRbr force-pushed the upgrade-oclif branch 4 times, most recently from 77dae75 to 44d05d9 Compare November 19, 2024 09:50
@paulRbr paulRbr changed the title maintenance: upgrade to TS 5 and Oclif 4 maintenance: upgrade to TypeScript 5, Oclif 4, node 18+ Nov 19, 2024
@paulRbr paulRbr force-pushed the upgrade-oclif branch 3 times, most recently from e9b2a42 to 5bafc68 Compare November 21, 2024 11:26
@paulRbr paulRbr marked this pull request as ready for review November 21, 2024 11:31
src/commands/deploy.ts Outdated Show resolved Hide resolved
@Polo2
Copy link
Member

Polo2 commented Nov 21, 2024

feedbacks following live test:

  • node v20 is required as minimal version

bump preview: all good
option -o does not work, IMO it can be removed

bump preview: all good

bump deploy: all good
in hub, when interactive mode is selected and api filename should be removed, I have an error (only when flag --auto-create was missing, maybe a false positive)

bump diff : all good

bump overlay: all good

I'm very impressed by this new version of CLI, well done 👏

This is an upgrade of the package description to use a modern version
of Typescript and use the latest v4 of oclif.

It's basically a whole re-write from a scratch generated oclif package.
@paulRbr
Copy link
Member Author

paulRbr commented Nov 27, 2024

in hub, when interactive mode is selected and api filename should be removed, I have an error (only when flag --auto-create was missing, maybe a false positive)

As discussed in sync, there was a bug when selecting multiple times the same file: I've fixed the code to only present files that have not yet been selected (and thus the error should not be possible anymore)

  • node v20 is required as minimal version

Changed the minimal version in the package.json ✅

@paulRbr
Copy link
Member Author

paulRbr commented Nov 27, 2024

@Polo2 could you take a second look and if happy approve the PR 🙏?

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.

Fully migrate to @oclif/core since @oclif/command is now deprecated
2 participants