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.

Sign up for GitHub

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

Jump to bottom

fix: support TS resolution in JS files when allowJs is set #535

Merged
merged 4 commits into from
Apr 25, 2024
Merged

fix: support TS resolution in JS files when allowJs is set #535

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1632a30
Support TS resolution in JS files when allowJs is set
nwalters512 Apr 23, 2024
86e33ea
Fix test and CJS behavior
nwalters512 Apr 24, 2024
c80ef6c
Merge branch 'develop' into support-allowjs
privatenumber Apr 24, 2024
f4484fc
Merge branch 'develop' into support-allowjs
privatenumber Apr 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 4 additions & 1 deletion src/cjs/index.ts
View file
Open in desktop
Expand Up @@ -205,7 +205,10 @@ const resolveTsFilename = (

if (
parent?.filename
&& isTsFilePatten.test(parent.filename)
&& (
isTsFilePatten.test(parent.filename)
|| tsconfig?.config.compilerOptions?.allowJs
)
&& tsPath
) {
for (const tryTsPath of tsPath) {
Expand Down
13 changes: 6 additions & 7 deletions src/esm/loaders.ts
View file
Open in desktop
Expand Up @@ -17,6 +17,7 @@
isJsonPattern,
getFormatFromFileUrl,
fileProtocol,
allowJs,
type MaybePromise,
type NodeError,
} from './utils.js';
Expand Down Expand Up @@ -133,7 +134,7 @@

const isRelativePathPattern = /^\.{1,2}\//;

export const resolve: resolve = async (

Check warning on line 137 in src/esm/loaders.ts

View workflow job for this annotation

GitHub Actions / Test (ubuntu-latest) 

Async arrow function has a complexity of 23. Maximum allowed is 10
specifier,
context,
defaultResolve,
Expand Down Expand Up @@ -166,13 +167,11 @@
}
}

/**
* Typescript gives .ts, .cts, or .mts priority over actual .js, .cjs, or .mjs extensions
*/
if (
// !recursiveCall &&
tsExtensionsPattern.test(context.parentURL!)
) {
// Typescript gives .ts, .cts, or .mts priority over actual .js, .cjs, or .mjs extensions
//
// If `allowJs` is set in `tsconfig.json`, then we'll apply the same resolution logic
// to files without a TypeScript extension.
if (tsExtensionsPattern.test(context.parentURL!) || allowJs) {
const tsPaths = resolveTsPath(specifier);
if (tsPaths) {
for (const tsPath of tsPaths) {
Expand Down
1 change: 1 addition & 0 deletions src/esm/utils.ts
View file
Open in desktop
Expand Up @@ -19,6 +19,7 @@ const tsconfig = (

export const fileMatcher = tsconfig && createFilesMatcher(tsconfig);
export const tsconfigPathsMatcher = tsconfig && createPathsMatcher(tsconfig);
export const allowJs = tsconfig?.config.compilerOptions?.allowJs ?? false;

export const fileProtocol = 'file://';

Expand Down
19 changes: 19 additions & 0 deletions tests/specs/smoke.ts
View file
Open in desktop
Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

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

I didn't fully understand how the tests are structured/split up; I'd be more than happy to adjust things here in response to feedback.

Copy link
Owner

Choose a reason for hiding this comment

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

For context, the idea behind the smoke tests here is to test with minimal number of child processes; the other tests are slow primarily because it spawns so many children.

Expand Up @@ -218,6 +218,14 @@ const files = {

'file.txt': 'hello',

'import-typescript-parent.js': sourcemap.tag`
import './import-typescript-child.js';
Copy link
Owner

Choose a reason for hiding this comment

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

Can you move this inside import-from-js.js?

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

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

I can, though that will require adding --tsconfig tsconfig/tsconfig-allowJs.json to that test's arguments, at which point I think there wouldn't be any tests that cover the case where TS resolution rules aren't followed. If that's good with you, I'll do the move and delete the allowJs in tsconfig.json test.

Copy link
Owner

Choose a reason for hiding this comment

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

Ah good point, let's keep it this way then. Thanks for catching it!

`,

'import-typescript-child.ts': sourcemap.tag`
console.log('imported');
Copy link
Owner

Choose a reason for hiding this comment

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

Make this allowJs or something more unique that can be matched from within the 'from .js' test

`,

node_modules: {
'pkg-commonjs': {
'package.json': JSON.stringify({
Expand Down Expand Up @@ -663,6 +671,17 @@ export default testSuite(async ({ describe }, { tsx }: NodeApis) => {
expect(pTsconfigAllowJs.stderr).toMatch('Error: No error thrown');
expect(pTsconfigAllowJs.stdout).toBe('');
});

test('allowJs in tsconfig.json', async ({ onTestFail }) => {
const pTsconfigAllowJs = await tsx(['--tsconfig', 'tsconfig/tsconfig-allowJs.json', 'import-typescript-parent.js'], fixture.path);
onTestFail((error) => {
console.error(error);
console.log(pTsconfigAllowJs);
});
expect(pTsconfigAllowJs.failed).toBe(false);
expect(pTsconfigAllowJs.stderr).toBe('');
expect(pTsconfigAllowJs.stdout).toBe('imported');
});
});
});
}
Expand Down