Skip to content

Drop Some Dependencies - #54

Closed
avivkeller wants to merge 1 commit into
nodejs:mainfrom
avivkeller:drop-chalk
Closed

Drop Some Dependencies#54
avivkeller wants to merge 1 commit into
nodejs:mainfrom
avivkeller:drop-chalk

Conversation

@avivkeller

@avivkelleravivkeller commented Apr 13, 2024

Copy link
Copy Markdown
Member

This PR drops a few dependencies to make this package smaller. This will help with the eventual movement to include this in NodeJS core.

This PR does NOT drop emphasize (5.5MB)

See nodejs/node#52510

@avivkelleravivkeller changed the title Drop Chalk DependencyDrop Some DependenciesApr 13, 2024
@avivkeller

Copy link
Copy Markdown
MemberAuthor

After some hard work, I managed to (in my PR):

Drop Dependencies (The main goal)

  1. Drop chalk (Replace with util.inspect.colors) Note that chalk is still pre-installed with emphasize

  2. Drop strip-ansi (Replace with basic implementation)

  3. Drop ws (Replace with inspector) Note that this change changes a lot, and may be unstable

  4. Drop child_process (Replace with inspector)

    Fixes "if the child process (which runs all the code) becomes frozen it will not be killed after the parent exits."

Little Fixes

These just came up when I was updating the codebase

  1. Fix multi-line preview

@avivkeller

Copy link
Copy Markdown
MemberAuthor

/rr @devsnek
Requesting review

@VoltrexKeyva

Copy link
Copy Markdown

What is the reason for removing the development dependencies (devDependencies)?

@avivkeller

Copy link
Copy Markdown
MemberAuthor

What is the reason for removing the development dependencies (devDependencies)?

Oops! That must've been my bad, sorry! I'll fix it now

@devsnek

Copy link
Copy Markdown
Member

Drop child_process (Replace with inspector)

why? using child_process is very intentional, it allows the interface to recover from bad logic that blocks evaluation.

@avivkeller

Copy link
Copy Markdown
MemberAuthor

Drop child_process (Replace with inspector)

why? using child_process is very intentional, it allows the interface to recover from bad logic that blocks evaluation.

As far as I could tell, child_process was only used to init the REPL, so IMO, using the inspector to init the REPL seems like a better idea, as it is what is used in the native NodeJS. If you disagree, I can add it back.

@avivkeller

avivkeller commented Apr 13, 2024

Copy link
Copy Markdown
MemberAuthor

The latest commits allows the preview of the line to preview based on the autofill, similar to how nodejs does it.
(mo -> preview module)

@avivkeller

Copy link
Copy Markdown
MemberAuthor

@devsnek, if you remember any other bugs, I can work on fixing them as well (if you don't mind)

@avivkeller

Copy link
Copy Markdown
MemberAuthor

If I find any other bug fixes, would you prefer I stash them locally and wait for this PR to be resolved, or just push them into this?

@avivkeller

Copy link
Copy Markdown
MemberAuthor

Hi, @devsnek; sorry for bugging you (again), but how are you feeling about this PR. I'd love some feedback so I can make it (and my other PRs) better for the future!

@avivkelleravivkeller closed this by deleting the head repository Apr 24, 2025
Sign up for freeto 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.

3 participants

@avivkeller@VoltrexKeyva@devsnek