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

Add support for .cjs and .mjs input files #39840

Conversation

ExE-Boss
Copy link
Contributor

@ExE-Boss ExE-Boss commented Jul 30, 2020

I got fed up with not having IntelliSense or type checking in .cjs and .mjs files, so I’ve decided to fix this myself.

@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Jul 30, 2020
@ExE-Boss ExE-Boss force-pushed the feat/add-support-for-cjs-and-mjs-input-files branch from 92c08fd to cc3f921 Compare July 30, 2020 21:54
@jkrems
Copy link

jkrems commented Jul 31, 2020

Sorry to say that but your implementation is not spec compliant

Those examples seem to be about the resolver features, not about the file extensions. The resolver is equally "broken" for .js right now, unless I'm missing something?

@frank-dspeed
Copy link

frank-dspeed commented Jul 31, 2020

@jkrems you are partial right at present it is as broken as typescript is broken it self.
But the most interristing part is upcoming we have landed for example TLA inside NodeJS

I can not explain all that in a short comment or issue but this result is the same as we got befor 3 years :).

@frank-dspeed
Copy link

I think this issue can finaly not get fixed in this way typescripts module system needs refactor much more then accepting .mjs and cjs as extension for js handling.

@jkrems
Copy link

jkrems commented Jul 31, 2020

I think this issue can finaly not get fixed in this way typescripts module system needs refactor much more then accepting .mjs and cjs as extension for js handling.

I think you are right that a more fundamental fix may be desirable. The question is: If just letting people import ./my-file.mjs in vscode and get proper auto-complete is blocked on that and this PR makes the auto-complete work (for people who potentially never touch Typescript), is it fair to delay these quality-of-life fixes until such a fundamental fix is ready? So far it doesn't sound like there's resources and/or a timeline for a major redo of the TS module system. At least that seemed to be the public communication.

So what is the path forward short term, if not a patch like this? People who want to write packages that contain (some) native ESM should stop using vscode to do it? They should use some community fork with these changes and override the project TS (which the ts language server might then pick up..?)?

@frank-dspeed
Copy link

frank-dspeed commented Jul 31, 2020

@jkrems there exists a other workaround without recode that enables that
assume this files

index.d.ts

// interface other typescript types export import
// can also export {}
// that means simply any.

myapp.mjs

import some from 'some-other'

myapp.mjs.d.ts

// This is typescript syntax it can now import .ts .d.ts and .js types and rexport them
export * as types from './index.d.ts'
export = types

The export = is correct typescript syntax it makes now myapp.mjs import able and it will show it with correct types if you want to even import it with specifier as you would do in nodejs you can create additional

myapp.d.ts

// This is typescript syntax it can now import .ts .d.ts and .js types and rexport them
export * as types from './index.d.ts'
export = types

Final usage

import myapp from './myapp' //will lookup myapp.d.ts while nodejs will resolve myapp.mjs all works
import myapp from './myapp.mjs' //will lookup myapp.mjs.d.ts all works

@frank-dspeed
Copy link

frank-dspeed commented Jul 31, 2020

@jkrems and the path forward is clear drop typescripts extension .ts and replace it with solutions like assert calls and jsdoc typechecking 👍 and also a flow like Comment Syntax maybe contributed to JSDoc if that does not exist

@jkrems
Copy link

jkrems commented Jul 31, 2020

I'm not sure how eslint relates to autocomplete. The following currently works in vscode without any .d.ts or other Typescript files involved:

// b.js
export default class Foo { bar() {} }

// a.js
import Foo from './b.js';
new Foo(). // <suggests bar() as an available method>

It doesn't work if b.js is renamed to b.mjs or b.cjs (the latter with the respective syntax change to CommonJS). No amount of eslint can fix that afaik. And writing exhaustive manual .d.ts files for every JavaScript file doesn't sound like a practical workaround.

@ExE-Boss
Copy link
Contributor Author

@frank-dspeed

Any way as all this is of topic and i will solve it at a other front i only wanted to point out that this is the same patch that was already posted in the issue befor but now ported for current typescript.

That is incorrect, as that patch only hacks together a way for the TypeScript language server to recognise the .mjs file extension as a JavaScript file, but everything else is still broken with it.


This PR adds proper validation, import auto‑complete and enforces that .cjs files always parse as CommonJS modules and .mjs files always parse as ECMAScript modules.

As for the package.json exports field, that’s issue #33079, which is out of scope for this PR.

@frank-dspeed
Copy link

@ExE-Boss do not get me wrong it is a good thing but microsoft will not merge it we would need to maintain a vscode fork for that and i am not joking i am fighting with them since 5+ years.

Your Solution is better then nothing. And it is more then i got done till today :) So i already cherry picked it internal.

but we need to make it more easy useable as sayed overall the path forward is to create or fork the deprecated language-server project then integrate for example this typescript version and create a installer for that so that existing vscode users can simple install this as additional language server
then we could start to solve the problem complet via refactoring this indipendent.

@bnb
Copy link
Contributor

bnb commented Aug 1, 2020

Just wanted to say thank you for doing this work @ExE-Boss 🙏🏻

@F1LT3R
Copy link

F1LT3R commented Aug 1, 2020

I got fed up with not having IntelliSense or type checking in .cjs and .mjs files, so I’ve decided to fix this myself.

* Fixes #27957

* Fixes #38784

@ExE-Boss You will be remembered as a god if you pull this off.

@frank-dspeed
Copy link

frank-dspeed commented Aug 7, 2020

If some one does want to test this https://github.com/stealify/typescript

npm i -g vscode-typescript@https://github.com/stealify/typescript
# Getting global path
npm root -g

Hit f1 on keyboard open user settings and set

"typescript.tsdk": "{your_global_npm_path}/vscode-typescript/lib"

restart is not needed.

@ExE-Boss
Copy link
Contributor Author

ExE-Boss commented Aug 9, 2020

  1. should resolve './module-with-dts.mjs', './module-with-dts.mjs.d.ts' if they exist
import './module-with-dts' 

That is the intended behaviour.

@ExE-Boss
Copy link
Contributor Author

ExE-Boss commented Aug 11, 2020

@frank-dspeed That’s because all engines (including Node.js) forbid loading native ES Modules without the file extension.

// This will fail at runtime if `module-with-dts` doesn't exist in the same directory,
// even if `module-with-dts.js` (or `module-with-dts.mjs`) does:
import "./module-with-dts";

Note that this PR makes it so that ./module‑with‑dts.mjs can resolve types from ./module‑with‑dts.d.ts, just like ./module‑with‑dts.js does.

@frank-dspeed
Copy link

frank-dspeed commented Aug 11, 2020

@ExE-Boss that is not true with the extension part we got a new node --experimental-specifier-resolution=node index and in general we support that all over in v8 and in deno and in node-graaljs we do allow specifiers for esm all over the place inside the ecosystem but ok anway i now understand the full impact of that pull request

I will go ahead then now with rollup-language-server to solve all that compat issues once and for ever with and without typescript 👍 thanks for your effort.

Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

We're not going to add .cjs and .mjs extensions until we have a complete plan for supporting them.

@sandersn
Copy link
Member

sandersn commented Sep 3, 2020

A few years ago when I was working on early support for .vue files, I had pretty good luck with manually telling emacs to treat arbitrary extensions as Typescript. That might still work, although probably intra-file support won't be great.

@ExE-Boss ExE-Boss marked this pull request as draft September 4, 2020 16:02
@bnb
Copy link
Contributor

bnb commented Sep 4, 2020

@sandersn for context, I'm interested in this support landing since it's required for VS Code to automatically update import paths with .mjs and .cjs. I'm not particularly looking for that support for myself but for the ecosystem as someone who works on Node.js and cares about ESM in Node.js and a smooth/low-barrier ecosystem transition.

@frank-dspeed
Copy link

UPDATE* At present i expirence no down sides in the EDITOR with such Changes even not with my version that sets everything to .js

@sandersn sandersn added the Experiment A fork with an experimental idea which might not make it into master label Oct 27, 2020
@typescript-bot
Copy link
Collaborator

The TypeScript team hasn't accepted the linked issue #27957. This makes it less likely that we'll review or accept this PR. Try to get the originating issue accepted.

@ExE-Boss
Copy link
Contributor Author

Fixed in #45884 (TypeScript 4.5)

@ExE-Boss ExE-Boss closed this Oct 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Experiment A fork with an experimental idea which might not make it into master For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support type checking .cjs files Support ".mjs" input files
7 participants