nteract / outputs

A collection of React components for displaying rich Jupyter display objects
BSD 3-Clause "New" or "Revised" License
25 stars 19 forks source link

RFC: Ansi output options #99

Open brism17 opened 2 years ago

brism17 commented 2 years ago

Nteract desktop app and jupyter extension currently uses the ansi-to-react library that takes four props to configure the ansi outputs. I want to configure the passing of optional options to the ansi object the controls stream and error outputs.

Motivation

Azure notebooks needs to change some of the default ansi values to fit our customers' needs. We understand that what is best for our product might not be what is best for other consumers of nteract. Making these options configurable will help all nteract consumers create the best experience to fit their needs.

Goals:

Design and Implementation

Ansi to react has four properties that are passed down as props found here

 declare interface Props {
        children?: string;
        linkify?: boolean;
        className?: string;
      useClasses?: boolean;
  }

I want to add two of these options: useClasses and Linkify, to be passed down via the kernel-output-error class and the stream-text.tsx so that individual consumers have the power to control how ansi outputs are rendered. These options will be added to the existing props for these output classes. The following is kernel-ouput-error:

   interface Props {
     className?: string;
     output: ImmutableErrorOutput;
     output_type: "error";
     linkify?: boolean;
     useClasses?: boolean;
  }

The following is stream-text:

  interface Props {
      output_type: "stream";
      output: ImmutableStreamOutput;
      linkify?: boolean;
      useClasses?: boolean;
   }

These options would override the default properties if provided which would match the current options in nteract.

Testing

To validate the correct behavior of the new ansi options I will add unit tests for the different options configurations and for if no options are provided..

Compatibility

If no options are provided I will provide default values that are the same as the current settings in nteract.

User Impact

No user impact if a user doesn't pass down options, the current behavior will stay the same. However, if a user chooses to pass down options they will have full control of how ansi outputs are rendered.

captainsafia commented 2 years ago

Azure notebooks wants to make links in error outputs clickable.

Hm. Does this not work at the moment or would you like to be able to conditionally enable or disable it?

These options will be added to the existing props for these output classes.

Is there a chance we can add them under a new AnsiOptions option so that we have the ability to manage this type independently?

Where are AnsiOptions stored? In the app state?

brism17 commented 2 years ago

Azure notebooks wants to make links in error outputs clickable.

This doesn't work at the moment and the goal of this RFC is to be able to conditionally enable it. The linkify option for ansi is set to false here. Instead of false being hard coded it would read from a prop value passed down through kernel-output-error by nteract's consumers.

Is there a chance we can add them under a new AnsiOptions option so that we have the ability to manage this type independently?

I thought about doing this initially but since there were only two props I wanted to add I thought it would be fine to just add them to the list of existing props for stream-text and kernel-output-error. The options are just stored as props in the react object and passed down to the Ansi class when its initialized. There is no reason for these options to need to be stored in state. Another reason why I didn't make an ansiOptions prop is because kernel-output-error already has one of the ansi props (className) that it passes down to the ansi object. I don't want to add any breaking changes and it wouldnt make sense for one of the ansi props to be outside of the ansiOptions object to make this change .