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

feat(core): read–eval–print loop feature #9684

Merged
merged 30 commits into from
Jun 15, 2022
Merged

feat(core): read–eval–print loop feature #9684

merged 30 commits into from
Jun 15, 2022

Conversation

kamilmysliwiec
Copy link
Member

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

https://twitter.com/i/status/1529088190958735361

repl2

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@coveralls
Copy link

coveralls commented May 29, 2022

Pull Request Test Coverage Report for Build b52928ac-3c6e-4a55-a0d4-5db25ea40e60

  • 116 of 124 (93.55%) changed or added relevant lines in 10 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.01%) to 94.096%

Changes Missing Coverage Covered Lines Changed/Added Lines %
packages/common/utils/cli-colors.util.ts 0 1 0.0%
packages/core/repl/native-functions/debug-repl-fn.ts 22 23 95.65%
packages/core/repl/repl-context.ts 48 51 94.12%
packages/core/repl/repl-function.ts 4 7 57.14%
Totals Coverage Status
Change from base Build 6a42a282-8488-456e-9dd2-df532926a28e: -0.01%
Covered Lines: 5897
Relevant Lines: 6267

💛 - Coveralls

@Tony133
Copy link
Contributor

Tony133 commented May 29, 2022

A fantastic feature!. I can't wait to try it out 😻

@micalevisk
Copy link
Member

can't we have a .exit command (like Nodejs REPL) to quit the REPL? 🤔

@Tony133
Copy link
Contributor

Tony133 commented May 29, 2022

I think Ctrl + C should be enough to exit or exit() it would be perfect ,but I don't know if the nest application is always running once REPL is closed 🤔

@micalevisk
Copy link
Member

micalevisk commented May 29, 2022

as we have debug(), exit() would be better (an alternative for those who can't use Ctrl+c)

@Tony133
Copy link
Contributor

Tony133 commented May 29, 2022

Many times if not always Ctrl + c is more convenient 😁

@micalevisk
Copy link
Member

micalevisk commented May 29, 2022

I just test this and we already has .exit (and others) due to repl package from nodejs

image

It didn't work as Ctrl+c tho

@jmcdo29 jmcdo29 mentioned this pull request May 29, 2022
@micalevisk
Copy link
Member

micalevisk commented May 29, 2022

I have three suggestions that I believe could improve DX:

  1. Allow consumers to supply their own class that extends the ReplContext one, instead of hard-coding new ReplContext(app) on repl function
  2. Display help messages by calling <command>.help (or by any other means)
  3. Have some way to display the built-in expressions added by NestJS. I mean, we would have to go to the docs when forgot what are the functions available and what they do (debug, methods, etc). Maybe a help() that display a list of all other functions would be enough

I've implemented both locally to demonstrate 1 and 2:

Implementation

image

parts of the code
// repl-context.ts
export const REPL_COMMANDS = 'repl:all-commands';

type HelpTextBuilder = (colors: typeof clc) => string;

export function ReplCommand(
  helpTextOrBuilder: string | HelpTextBuilder,
): MethodDecorator {
  return (target: Object, propertyKey: string | symbol) => {
    const commands =
      Reflect.getMetadata(REPL_COMMANDS, target.constructor) || [];
    commands.push(propertyKey);
    Reflect.defineMetadata(REPL_COMMANDS, commands, target.constructor);

    Object.defineProperty(target[propertyKey], 'helpText', {
      value:
        (typeof helpTextOrBuilder === 'string'
          ? helpTextOrBuilder
          : helpTextOrBuilder(clc)) + '\n',
      enumerable: false,
      writable: false,
      configurable: false,
    });
  };
}
// repl.ts
import { Logger, Type } from '@nestjs/common';
import * as _repl from 'repl';
import { NestFactory } from '../nest-factory';
import { REPL_INITIALIZED_MESSAGE } from './constants';
import { ReplContext, REPL_COMMANDS } from './repl-context';
import { ReplLogger } from './repl-logger';

interface ReplOptions {
  Context?: Type<ReplContext>;
}

export async function repl(
  module: Type,
  replOptions?: ReplOptions = {},
) {
  const app = await NestFactory.create(module, {
    abortOnError: false,
    logger: new ReplLogger(),
  });
  await app.init();

  const Context = replOptions.Context || ReplContext;
  const replContext = new Context(app);
  Logger.log(REPL_INITIALIZED_MESSAGE);

  const replServer = _repl.start({
    prompt: '\x1b[32m>\x1b[0m ',
    ignoreUndefined: true,
  });

  const commands: string[] = Reflect.getMetadata(REPL_COMMANDS, Context) || [];
  commands.forEach(command => {
    if (!replContext[command]) return;

    replServer.context[command] = replContext[command].bind(replContext);

    Object.defineProperty(replServer.context[command], 'help', {
      get: () => replContext.writeToStdout(replContext[command].helpText),
      enumerable: false,
      configurable: false,
    });
  });

  return replServer;
}
demo.mp4

@kamilmysliwiec
Copy link
Member Author

@micalevisk feel free to create a PR that's targeting this PR :)

@micalevisk
Copy link
Member

@kamilmysliwiec I guess we can postpone those suggestions as they won't break the current behavior (and could be trickier to write tests).

@kamilmysliwiec
Copy link
Member Author

@micalevisk I don't plan to merge it earlier than ~mid-June anyway so feel free to create a PR whenever you're ready!

@kamilmysliwiec kamilmysliwiec mentioned this pull request Jun 2, 2022
12 tasks
}

private initializeContext() {
const globalRef = globalThis;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why renaming globalThis to globalRef here but not in the introspectCollection method? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this might be a leftover I forgot to remove

Copy link
Contributor

@delucca delucca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing feature! Looking forward to use it 🚀

Just left minor comments regarding some typos :)

@@ -0,0 +1,6 @@
export * from './help-repl-fn';
export * from './get-relp-fn';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there is a minor typo here 😄

Suggested change
export * from './get-relp-fn';
export * from './get-repl-fn';

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, my bad. I fixed them in #9720

@@ -0,0 +1,17 @@
import type { Type } from '@nestjs/common';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(related to file): There is a minor typo in the file name. It should be get-repl-fn.ts

@kamilmysliwiec kamilmysliwiec changed the base branch from master to 9.0.0 June 15, 2022 09:40
@kamilmysliwiec kamilmysliwiec added this to the 9.0.0 milestone Jun 15, 2022
@kamilmysliwiec kamilmysliwiec merged commit b29d88f into 9.0.0 Jun 15, 2022
@delete-merged-branch delete-merged-branch bot deleted the feat/repl branch June 15, 2022 09:41
@kamilmysliwiec
Copy link
Member Author

@micalevisk would you like to start working on a PR to the docs (for this feature)? Preferably in Recipes category :) I can pick it up later on if needed!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants