Make ts-node eval public for node REPL consumption

[MarcManiez]

Fixes #983

I took the second approach suggested by @cspotcode in #983:

Alternatively, we expose only the eval implementation via a createReplEval function.
https://github.com/TypeStrong/ts-node/blob/master/src/bin.ts#L350-L353

That way you fully control construction of the REPL, simply passing it a ts-node eval implementation.

This necessitates moving quite a few things around. I've done my best to respect the conventions I could discern, but please let me know if I've missed something.

[codecov]

Codecov Report

Merging #1121 (5267c0b) into master (286c294) will increase coverage by 0.53%.
The diff coverage is 77.34%.

@@            Coverage Diff             @@
##           master    #1121      +/-   ##
==========================================
+ Coverage   79.33%   79.87%   +0.53%     
==========================================
  Files          11       12       +1     
  Lines         755      780      +25     
  Branches      157      165       +8     
==========================================
+ Hits          599      623      +24     
- Misses        101      102       +1     
  Partials       55       55              

Flag	Coverage Δ
node_10	77.02% <75.00%> (+0.81%)	⬆️
node_12_15	77.35% <76.56%> (+0.80%)	⬆️
node_12_16	77.35% <76.56%> (+0.80%)	⬆️
node_13	79.23% <76.56%> (+0.55%)	⬆️
node_14	79.23% <76.56%> (+0.55%)	⬆️
node_14_13_0	78.46% <76.56%> (+0.58%)	⬆️
node_15	79.23% <76.56%> (+0.55%)	⬆️
typescript_2_7	79.23% <76.56%> (+0.55%)	⬆️
typescript_latest	78.46% <76.56%> (+0.58%)	⬆️
typescript_next	78.46% <76.56%> (+0.58%)	⬆️
ubuntu	79.10% <76.56%> (+0.55%)	⬆️
windows	79.23% <76.56%> (+0.55%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/repl.ts	74.78% <74.78%> (ø)
src/bin.ts	90.24% <100.00%> (+11.05%)	⬆️
src/index.ts	79.25% <100.00%> (+0.05%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 286c294...5267c0b. Read the comment docs.

[coveralls]

Coverage increased (+1.07%) to 87.139% when pulling 58354c3 on MarcManiez:public-eval into 41e7109 on TypeStrong:master.

[cspotcode]

@MarcManiez thanks for tackling this. I hope to make a proper review today. If for some reason I forget by Monday, feel free to ping me about it.

I appreciate the effort to respect existing conventions even when you had to move so much around.

Our existing API has a create() function that creates the ts-node service, then returns an object exposing everything about the service. Should we take the same approach for the REPL? Something like this?

import {create, createReplService} from 'ts-node';
const tsNodeService = create(/* options here */);
const tsNodeReplService = createReplService(tsNodeService);
const {eval} = tsNodeReplService;

Also, I don't know how @blakeembrey feels about this, but I'm in favor of moving the REPL functionality to a new file: repl.ts.
When concision sent us a few PRs recently, they mentioned that it was a bit tough to jump into the code with almost everything in index.ts. If we can, splitting things out might help new contributors.

[MarcManiez]

Thanks for taking a peep @cspotcode :)

I took a stab at what you suggested with createReplService: 9ab7eca. Did you see that including any properties other than eval?

I also moved the REPL code to its own file like you suggested. Happy to revert it if it's not to everyone's liking. From my newcomer's perspective, I agree with concision in that splitting the code into more files could help understand the code's structure a little better.

[cspotcode]

Very cool, I'll take a look later. Are you comfortable with me pushing changes to your branch when I review? Sometimes there are small nitpicks I can fix, or I can propose changes that we can always revert if there's disagreement. I figure it's easier than asking you to do all the work.

…

On Fri, Sep 4, 2020, 6:30 PM Marc Maniez ***@***.***> wrote: Thanks for taking a peep @cspotcode <https://github.com/cspotcode> :) I took a stab at what you suggested with createReplService: 9ab7eca <9ab7eca>. Did you see that including any properties other than eval? I also moved the REPL code to its own file like you suggested. Happy to revert it if it's not to everyone's liking. From my newcomer's perspective, I agree with concision in that splitting the code into more files could help understand the code's structure a little better. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1121 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAC35OCWVCQGWVYSVCQOVT3SEFTBLANCNFSM4QWJL2DQ> .

[MarcManiez]

Absolutely no worries with that, thanks for checking in!

[NickSeagull]

Hello! Any progress on this? I was about to make a fork in order to allow exactly this, and would like to use this feature in my current project. Would like to help on what it would be needed :)

[JDvorak]

Looking forward to this! Thank you @MarcManiez

[cspotcode]

No blockers, no update. I've had a busy week with other priorities. In the meantime, you can install ts-node directly off of a git branch using npm's git support. So you can start installing and using this feature today. And when we are able to publish and merge it (in the next couple weeks I expect) you can switch from the git dependency back to the npm package.

…

On Mon, Sep 14, 2020, 12:40 PM John S. Dvorak ***@***.***> wrote: Looking forward to this! Thank you @MarcManiez <https://github.com/MarcManiez> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1121 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAC35OE7ONKQF4IS6BYET6TSFZBQDANCNFSM4QWJL2DQ> .

[cspotcode]

I am finally getting around to reviewing this. I'm not quite done yet. Because of the refactoring, which I think is a good thing, it's taking me a tad longer to verify that I understand all the changes and that nothing has accidentally broken when merging upstream changes from master.

[MarcManiez]

That's exciting, thank you for handling the merge 🙏

I see codecov/patch is off the mark. If you have advice on how to best address that, I'm happy to help!

[cspotcode]

I'm not sure why codecov/patch is off. Hopefully it ... fixes itself by the time we merge?

I reordered the declarations in repl.ts to match their ordering from bin.ts, just because it make it easier for me to review.

I also wanted to see if I could reduce coupling between repl.ts and bin.ts. So I expanded the API of ReplService and used the new API surface in bin.ts. I realize you may have wanted to do this yourself and tried to keep your changes more minimal, which I definitely appreciate.

I'm not 100% done -- there are a couple TODOs in the code and I need to add tests for the new APIs -- but if you feel like taking a look, I welcome any review feedback.

[cspotcode]

I think this is good enough to merge, pending a review. Added a test and a .md file with some very brief docs.

I don't love the naming on EvalAwarePartialHost.

Some of the @internals could be removed by moving those functions within the body of createReplService, but I'm ok being a bit lazy and deferring that work to a future PR unless anyone disagrees.

[Gerrit0]

This looks off - should it be tsNodeReplService.getStateAwareHostFunctions()?

[MarcManiez]

Looks like getStateAwareHostFunctions was left over from the previous commit which renamed getStateAwareHostFunctions to createEvalAwarePartialHost. So maybe this was the intent?

Suggested change

	const {readFile, fileExists} = repl.getStateAwareHostFunctions()
	const {readFile, fileExists} = tsNodeReplService.evalAwarePartialHost

[MarcManiez]

As a newcomer, I experienced some confusion seeing the replService juxtaposed with the service. I noticed that the Register type applies to the service and that naming discrepancy was also a head-scratcher for me. With some digging, I kind of see what they represent.

Nothing immediately applicable, but just sharing some thoughts, as I recall y'all considering what might make the repo easier on newbies!

[cspotcode]

Yeah, I don't like the name of Register. IMO it should be Service so that if you do import {Service} from 'ts-node'; then it's intuitive. I'm guessing the Register name made more sense in the past. We can rename it to Service and export by the legacy Register name for backwards-compatibility.

[cspotcode]

Another question:
Given that our create() function returns a Register, which I'd like to rename to Service, does it make sense to rename createReplService() to createRepl() for consistency? create(): Service, createRepl(): ReplService?

[MarcManiez]

Yes, I think that would make a lot more sense! I might even rename the ReplService type to TsRepl, or TsNodeRepl.

[cspotcode]

I'm thinking it's best to avoid putting TsNode into the names of exports, to match TypeScript and node's APIs. For example:

• import {Duplex} from 'streams'; (it's called Duplex, not DuplexStream)
• import {LanguageService} from 'typescript'; It's named LanguageService instead of TsLanguageService, and lots of usages I've seen do a wildcard import to add the ts prefix: import * as ts from 'typescript'; ts.LanguageService

[cspotcode]

Anybody know why the tests are failing on Windows? Maybe related to #1126

[MarcManiez]

🤯