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

Create ux.notify plugin or native command in oclif #1301

Closed
AllanOricil opened this issue Feb 17, 2024 · 6 comments
Closed

Create ux.notify plugin or native command in oclif #1301

AllanOricil opened this issue Feb 17, 2024 · 6 comments

Comments

@AllanOricil
Copy link

AllanOricil commented Feb 17, 2024

Do you want to request a feature or report a bug?

feature, and I can implement it if you find people who would want it.

What is the current behavior?

some long commands can usually take a lot of time to run, and developers would like to leave them running and come back only when a notifications appears. Some use cases:

  • synchronous salesforce metadata deployment using sf/sfdx
  • synchronous data processing/uploading jobs while using streams (I'm talking about files having more than the node runtime mem can handle) on large files

What is the expected behavior?

Instead of every single cli developer create its own notification system, I thought that this could be an oclif native, or plugin, module that extends the ux core module. Currently there is no ux.notify command in the available api.

https://github.com/oclif/core/tree/main/src/cli-ux

ux.notify("message");
// or
ux.notify("message",  { ...optional_props });
  • notify receives a message and optional properties to configure its behavior.
  • notify works on any OS

Possible solution

suggested library:
https://www.npmjs.com/package/node-notifier

vulnerabilities:
https://security.snyk.io/package/npm/node-notifier

optional props are just the props that node-notifier uses:

{
    title: 'My awesome title',
    icon: path.join(__dirname, 'coulson.jpg'), // Absolute path (doesn't work on balloons)
    sound: true, // Only Notification Center or Windows Toasters
    wait: true // Wait with callback, until user action is taken against notification, does not apply to Windows Toasters as they always wait or notify-send as it does not support the wait option
}

Maybe just a whole wrapper around node-notifier would already suffice.

@AllanOricil
Copy link
Author

forcedotcom/cli#2730

@AllanOricil
Copy link
Author

@mdonnalley
Copy link
Contributor

@AllanOricil The ux module contains a lot of wrappers around existing libraries (e.g progress, url, annotate) and a lot of half-baked versions of existing libraries (e.g. prompt, table, spinner) - neither of which are great for developers or for us as maintainers.

And so one of my goals in the next major version of oclif/core is to remove the majority of the methods on ux and ask developers to start using more specialized packages instead.

I mention this because, I don't think that adding a notify method to ux aligns with the long term vision. If people want notifications, then they can use a library like node-notifier directly instead of relying on oclif/core as a middle man.

That being said, I'm going to close this as not planned. But that doesn't mean that we won't end up doing this for sf - just that we wouldn't implement it in oclif/core

@mdonnalley mdonnalley closed this as not planned Won't fix, can't repro, duplicate, stale Feb 19, 2024
@AllanOricil
Copy link
Author

AllanOricil commented Feb 19, 2024

I think you could make ux a standalone package that bundles all those vendor packages, and your oclif compatible functions. A reliable way of ensuring oclif can interact with those libraries.
Then you can remove ux from the core bundle, add deprecation warnings for all ux methods in the next minor release, and when the new major releases you remove ux module entirely from core.

I would implement notify in it anyways because it is a way of saying:

"Hey, this oclif module called @oclif/ux has all the ux compatible libraries with oclif. You can of course use your desirable lib, but there is not garantee it will work"

Or in more technical words, but not quite

"Those vendor libraries were successfully integrated with oclif. It is safe to use on your cli"

This @oclif/ux would have to support tree shaking so that vendors and wrappers that were not used are not included in the final bundle.

You don't need to really maintain it, it will work as a way to ensure that oclif builds with @oclif/ux (integrates with no problem with it).


custom cli -> @oclif/core -> @oclif/ux   => WORKS AND WE ENSURE THAT

custom cli -> calling ux vendor directly => MAY NOT WORK

For example https://github.com/salesforce/lwc-test/blob/master/package.json#L32
I could use my own jest version to test lwc, but looking at this package I can be sure that specific version works with lwc-test

@mdonnalley
Copy link
Contributor

@AllanOricil That was the original design with the cli-ux package that we deprecated. We moved away from it mainly because @oclif/core and cli-ux were both dependent on each other which made development irritating. Since they both depended on each other, we decided to put them into the same package.

Regardless, I find it problematic to pre-select the packages that developers would like for their user experience since oclif is meant to be ux-agnostic.

"Hey, this oclif module called @oclif/ux has all the ux compatible libraries with oclif. You can of course use your desirable lib, but there is not garantee it will work"

Or in more technical words, but not quite

"Those vendor libraries were successfully integrated with oclif. It is safe to use on your cli"

What makes some libraries compatible with oclif and others not? I'm struggling to think of a reason why a library would be deemed "incompatible" with oclif.

@AllanOricil
Copy link
Author

What makes some libraries compatible with oclif and others not? I'm struggling to think of a reason why a library would be deemed "incompatible" with oclif.

@mdonnalley
I can't think of anything as well.

(cli) A -> B (@oclif/ux) -> C (vendor)

// if B does not do anything, then

(cli) A ----------------> C (vendor)

The only reasons that I could think for having B is if:

  1. B is doing something in the middle, before calling C.
  2. B wants to have control over the API. It can abstract and let devs implement it, or have a default implementation.

If 1 and 2 are not requirements then B is useless.

If you plan to get rid of it, it is better not to implement notify and add more complexity to something you are deleting.

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

No branches or pull requests

2 participants