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: create amaro loader #47

Merged
merged 3 commits into from
Aug 10, 2024
Merged

feat: create amaro loader #47

merged 3 commits into from
Aug 10, 2024

Conversation

marco-ippolito
Copy link
Member

@marco-ippolito marco-ippolito commented Aug 7, 2024

It is now possible to use amaro as external loader, that behaves the same way as in core, but allows to decouple the typescript version supported

package.json Outdated Show resolved Hide resolved
src/transform.ts Outdated Show resolved Hide resolved
@marco-ippolito
Copy link
Member Author

marco-ippolito commented Aug 8, 2024

This will behave exactly as the one in core, and it requires --experimental-strip-types to be enabled, so it uses node.js .ts resolution. This only overrides the swc transform function, and I would like to make it customizable.
If in the future typescript releases a major version, and has breaking changes, we might keep the amaro version in core locked. This allows users to update amaro and use it regardless of the node version

@marco-ippolito
Copy link
Member Author

PTAL @nodejs/typescript @nodejs/loaders

README.md Show resolved Hide resolved
src/loader.ts Outdated Show resolved Hide resolved
src/loader.ts Outdated Show resolved Hide resolved
src/loader.ts Outdated Show resolved Hide resolved
src/loader.ts Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/loader.ts Outdated Show resolved Hide resolved
src/loader.ts Outdated Show resolved Hide resolved
Copy link

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

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

We should either use a non-null assertion, we want to throw an error if the source is nullish. Falling back to an empty string is not doing a service to our users, as it's going to be much more confusing to debug. Alternatively, we could use a runtime assert if the non-null assert is not an option.

src/loader.ts Outdated Show resolved Hide resolved
src/loader.ts Show resolved Hide resolved
src/transform.ts Outdated Show resolved Hide resolved
@marco-ippolito
Copy link
Member Author

marco-ippolito commented Aug 9, 2024

@aduh95
Copy link

aduh95 commented Aug 9, 2024

There are cases in node core where source will be undefined https://github.com/nodejs/node/blob/3cbeed88d9de9231f420ba53ad0e4d4a7af9c637/lib/internal/modules/esm/get_format.js#L131-L136

IIUC that's only true for when format is nullish; the comment explains when defaultResolve returns a nullish format – and it doesn't matter for us, because Amaro will only deal with TS files

@marco-ippolito
Copy link
Member Author

marco-ippolito commented Aug 9, 2024

There are cases in node core where source will be undefined https://github.com/nodejs/node/blob/3cbeed88d9de9231f420ba53ad0e4d4a7af9c637/lib/internal/modules/esm/get_format.js#L131-L136

IIUC that's only true for when format is nullish; the comment explains when defaultResolve returns a nullish format – and it doesn't matter for us, because Amaro will only deal with TS files

Apparently it happens so I had to add the check here https://github.com/nodejs/node/blob/3cbeed88d9de9231f420ba53ad0e4d4a7af9c637/lib/internal/modules/helpers.js#L335

src/loader.ts Outdated Show resolved Hide resolved
@marco-ippolito marco-ippolito merged commit a712b1a into main Aug 10, 2024
8 checks passed
@marco-ippolito marco-ippolito deleted the feat/export-loader branch August 10, 2024 06:36
Sign up for free to 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.

8 participants