Skip to content

module: fix extensionless typescript in cjs loader - #54062

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
marco-ippolito:feat/fix-annoying-cjs-bug
Jul 29, 2024
Merged

module: fix extensionless typescript in cjs loader#54062
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
marco-ippolito:feat/fix-annoying-cjs-bug

Conversation

@marco-ippolito

@marco-ippolitomarco-ippolito commented Jul 26, 2024•
edited
Loading

Copy link
Copy Markdown
Member

Annoying bug that allowed extensionless typescript imports to work in cjs loader.

@nodejs-github-bot

nodejs-github-bot commented Jul 26, 2024

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/loaders

@nodejs-github-botnodejs-github-bot added the needs-ci PRs that need a full CI run. label Jul 26, 2024
anonrig
anonrig reviewed Jul 27, 2024
View reviewed changes
Comment threadlib/internal/modules/cjs/loader.js Outdated
@marco-ippolito
marco-ippolitoforce-pushed the feat/fix-annoying-cjs-bug branch 2 times, most recently from 5bbe964 to 75f97ceCompareJuly 27, 2024 05:28
@ChALkeRChALkeR mentioned this pull request Jul 27, 2024
Merged
ChALkeR
ChALkeR reviewed Jul 27, 2024
View reviewed changes
Comment threadlib/internal/modules/cjs/loader.js Outdated
ChALkeR
ChALkeR reviewed Jul 27, 2024
View reviewed changes
Comment threadlib/internal/modules/cjs/loader.js Outdated
ChALkeR
ChALkeR approved these changes Jul 27, 2024
View reviewed changes

@ChALkeRChALkeR left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM with the change above for .cts

@ChALkeR

ChALkeR commented Jul 27, 2024

Copy link
Copy Markdown
Member

Also, a test perhaps is missing for the .cts case

@marco-ippolito

marco-ippolito commented Jul 27, 2024•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

Ill add a test for cts later today

@marco-ippolito
marco-ippolitoforce-pushed the feat/fix-annoying-cjs-bug branch from 29a83e5 to 47589cdCompareJuly 28, 2024 07:59
@ChALkeR

ChALkeR commented Jul 28, 2024•
edited
Loading

Copy link
Copy Markdown
Member

Nice
This looks ready

@marco-ippolitomarco-ippolito added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 28, 2024
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 28, 2024
@nodejs-github-bot

nodejs-github-bot commented Jul 28, 2024

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/60689/

@atlowChemiatlowChemi added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 28, 2024
mcollina
mcollina approved these changes Jul 29, 2024
View reviewed changes

@mcollinamcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm

@nodejs-github-bot

nodejs-github-bot commented Jul 29, 2024

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/60714/

@nodejs-github-bot

nodejs-github-bot commented Jul 29, 2024

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/60725/

@nodejs-github-bot

nodejs-github-bot commented Jul 29, 2024

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/60728/

@marco-ippolitomarco-ippolito added the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 29, 2024
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 29, 2024
@nodejs-github-bot
nodejs-github-bot merged commit cf9a814 into nodejs:mainJul 29, 2024
@nodejs-github-bot

nodejs-github-bot commented Jul 29, 2024

Copy link
Copy Markdown
Collaborator

Landed in cf9a814

targos pushed a commit that referenced this pull request Jul 30, 2024
@marco-ippolito@targos
module: fix extensionless typescript in cjs loader
11aef40
PR-URL: #54062
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@RafaelGSSRafaelGSS mentioned this pull request Jul 30, 2024
Merged
RafaelGSS pushed a commit that referenced this pull request Aug 5, 2024
@marco-ippolito@RafaelGSS
module: fix extensionless typescript in cjs loader
ab1f0b4
PR-URL: #54062
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollinamcollinamcollina approved these changes
@ChALkeRChALkeRChALkeR approved these changes
@jasnelljasnelljasnell approved these changes
@benjamingrbenjamingrbenjamingr approved these changes
@anonriganonriganonrig approved these changes
@atlowChemiatlowChemiatlowChemi approved these changes

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.needs-ciPRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@marco-ippolito@nodejs-github-bot@ChALkeR@mcollina@jasnell@benjamingr@anonrig@atlowChemi@targos