FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

module: do not attempt to strip type when there's no source by aduh95 · Pull Request #54287 · nodejs/node · GitHub

/ node Public

module: do not attempt to strip type when there's no source - #54287

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
aduh95:simplify-type-stripping
Aug 12, 2024
Merged

module: do not attempt to strip type when there's no source#54287
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
aduh95:simplify-type-stripping

Conversation

aduh95 commented Aug 9, 2024

Copy link
Copy Markdown
Contributor

It bothers me that tsParse would return an empty string on non-string input, IMO it makes much more sense to use an assertion instead, and make sure the other parts of the code calling that function have already validated the input.

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/loaders

nodejs-github-bot added esm Issues and PRs related to the ECMAScript Modules implementation. module Issues and PRs related to the module subsystem. needs-ci PRs that need a full CI run. labels Aug 9, 2024
aduh95 requested a review from marco-ippolito August 9, 2024 16:45
marco-ippolito added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 9, 2024
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 9, 2024

Copy link
Copy Markdown
Collaborator

aduh95 added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Aug 9, 2024

Copy link
Copy Markdown
Collaborator

Copy link
Copy Markdown
Collaborator

Copy link
Copy Markdown
Collaborator

Copy link
Copy Markdown
Member

if #54250 lands we want to rethink this approach

aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 12, 2024

aduh95 commented Aug 12, 2024

Copy link
Copy Markdown
Contributor Author

if #54250 lands we want to rethink this approach

I don't see why, it's still a win to avoid calling tsParse

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 12, 2024
nodejs-github-bot merged commit 9a4eb21 into nodejs:main Aug 12, 2024

Copy link
Copy Markdown
Collaborator

Landed in 9a4eb21

aduh95 deleted the simplify-type-stripping branch August 12, 2024 09:16
targos pushed a commit that referenced this pull request Aug 14, 2024
PR-URL: #54287
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
RafaelGSS mentioned this pull request Aug 19, 2024
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. esm Issues and PRs related to the ECMAScript Modules implementation. module Issues and PRs related to the module subsystem. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants


Back | FazBrowse Home | New Git URL