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

ignored() bug: ESM loader will try to compile .cjs, .mjs, and other unexpected file extensions #1098

Closed
cspotcode opened this issue Aug 10, 2020 · 3 comments · Fixed by #1103
Labels

Comments

@cspotcode
Copy link
Collaborator

ESM loader uses our current ignored() implementation to determine if it should transform a file. This only excludes the .js and .jsx file extensions as needed. This worked fine with our CommonJS loader, because we knew we were only registered for the .ts, .tsx, and maybe .js and .jsx extensions.

The ESM loader is different, so it's asking ignore() about .mjs files. (maybe other extensions too?)

We need to update the ignored() implementation to include rather than exclude file extensions so that we only transform the same files that tsc can transform.

ts-node/src/index.ts

Lines 837 to 845 in ce7c323

const ignored = (fileName: string) => {
if (!active) return true
const relname = relative(cwd, fileName)
if (!config.options.allowJs) {
const ext = extname(fileName)
if (ext === '.js' || ext === '.jsx') return true
}
return !isScoped(relname) || shouldIgnore(relname)
}

@concision
Copy link
Contributor

concision commented Aug 10, 2020

Is it fine if I author a pull request for this?

I was thinking of reusing the conditions from the exported function getExtensions(config: _ts.ParsedCommandLine):

  const ignored = (fileName: string) => {
    if (!active) return true
    const ext = extname(fileName)
    const extensions = getExtensions(config)
    if (extensions.tsExtensions.includes(ext) || extensions.jsExtensions.includes(ext)) {
      const relname = relative(cwd, fileName)
      return !isScoped(relname) || shouldIgnore(relname)
    }
    return true
  }

Would this change be acceptable?

@cspotcode
Copy link
Collaborator Author

cspotcode commented Aug 10, 2020

Absolutely, we're happy for the help.

ignored() will be called a lot. Instead of calling getExtensions() within it, we can make the call outside of ignored. Maybe you were already planning to do this and the code you posted was a quick example.

I'm not sure if extname will handle .d.ts correctly. I'm not sure if it needs to, either. If we're trying to load a .d.ts file, then something has gone wrong elsewhere, right?

Other than that, looks good to me.

@concision
Copy link
Contributor

My thought process was to invoke getExtensions() from inside ignored(), in case someone wanted to monkey patch the exported function for some reason. I will make the call outside of ignored() since that is both unlikely and unconventional.

path.extname() seems to only retrieve the first level extension, so extname("typings.d.ts") resolves to .ts. Is there a scenario where secondary extensions (other than maybe .d.ts) should not be sent to the TypeScript compiler? This seems to be a non-issue with require.extensions, since the resolve algorithm will try to find the longest matching extension, allowing users to inject their own handler (e.g. require.extensions['.secondary.ts'] = ...). I suppose the responsibility should be passed on to the end user to order their ESM loaders correctly such that any secondary extensions are handled before reaching the ts-node ESM loader?

cspotcode added a commit that referenced this issue Aug 12, 2020
#1103)

* Fix #1098: `ignored()` bug: ESM loader will try to compile .cjs, .mjs, and other unexpected file extensions

* tests: Add test matrix for ignored() to ensure only specific extensions are sent to TypeScript compiler

* feat: Changed ESM getFormat to use a Register.ignored() lookup

* Add .d.ts extension to tests

Co-authored-by: Andrew Bradley <cspotcode@gmail.com>
@cspotcode cspotcode mentioned this issue Aug 21, 2020
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants