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

typescript build problem: node_modules/highlight.js/types/index.d.ts is not a module #2603

Closed
limbowang opened this issue Jun 12, 2020 · 42 comments

Comments

@limbowang
Copy link

limbowang commented Jun 12, 2020

ts file content:

// index.ts
import * as hljs from 'highlight.js';

after I run command

tsc index.ts

I got error

index.ts:1:23 - error TS2306: File '/Users/limbo/tmp/ssss/node_modules/highlight.js/types/index.d.ts' is not a module.

1 import * as hljs from 'highlight.js';
                        ~~~~~~~~~~~~~~


Found 1 error.

The version I use is 10.1.0.

Try to use 10.0.2 and it build successfully.

@joshgoebel
Copy link
Member

Not sure what you did exactly to get that error?

@joshgoebel
Copy link
Member

joshgoebel commented Jun 12, 2020

May need some TypeScript people to weigh in here... we only have the index.ts.d so we can check our own code in TS's checkJS mode - and so that perhaps it can be helpful to people using TS enabled editors for the code competition, etc...

It's possible we may need to change something slightly?

I'm not sure why TS would be trying to import it by default.

@joshgoebel
Copy link
Member

Is this a regression since 10.0? I don't think this has changed from 10.0 to 10.1

@limbowang
Copy link
Author

Is this a regression since 10.0? I don't think this has changed from 10.0 to 10.1

Have tried v10.0.x and it built successfully. There is no types/index.d.ts under v10.0.x

@joshgoebel
Copy link
Member

joshgoebel commented Jun 12, 2020

There is no types/index.d.ts under v10.0.x

Ah, so it was more recent... but I still don't know why your TS is trying to load that file? I don't think I understand how typescript loads libraries with import.

@joshgoebel
Copy link
Member

joshgoebel commented Jun 12, 2020

Maybe we need export = HLJSApi or something? Does it matter exactly what we export?

@limbowang
Copy link
Author

Suggest to use index.d.ts in @types/highlight.js. Or you can remove types: 'types/index.d.ts' in package.json. The ts compiler will read it by default.

@joshgoebel
Copy link
Member

The ts compiler will read it by default.

Only if it's in the root is my understanding from reading the docs.

@limbowang
Copy link
Author

limbowang commented Jun 12, 2020

A types package is a folder with a file called index.d.ts or a folder with a package.json that has a types field.

ref from https://www.typescriptlang.org/docs/handbook/tsconfig-json.html#types-typeroots-and-types

@plantain-00
Copy link

Same problems here.
Suggest to refer to export from index.d.ts in @types/highlight.js:

declare namespace hljs {
 ...
}
export = hljs;
export as namespace hljs;

https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/highlight.js/index.d.ts

@joshgoebel
Copy link
Member

joshgoebel commented Jun 13, 2020

If we just remove the types key from package.json will that solve this temporarily?

This obviously needs more thought.

@plantain-00
Copy link

Yes, it will fallback to @types/highlight.js.
But it may affect your own code checking.

@joshgoebel
Copy link
Member

joshgoebel commented Jun 13, 2020

I'm trying to understand the bigger picture here. It seems it would be much better if we bundled this type file (so we can keep it maintained) rather than @types/highlight.js having a very old version. But does this magic only work with npm?

I created a new TS project and put the build of HLJS in ./highlight.js, then I try to import it directly:

import hljs from './highlight.js';

This works fine (once I added the proper export to our index.d.ts file... It seems to know to use the index.ts.d file for the core import. But I can't import a language though:

import basic from './highlight.js/lib/languages/basic.js';

I get:

boot.ts:5:19 - error TS7016: Could not find a declaration file for module './highlight.js/lib/languages/basic.js'. '/Users/jgoebel/work/highlight.js/work/project/highlight.js/lib/languages/basic.js' implicitly has an 'any' type.

5 import basic from './highlight.js/lib/languages/basic.js';

This is despite having in the HLJS type file:

declare module 'highlight.js/lib/languages/*' {
  export default function(hljs?: HLJSApi): LanguageDetail;
}

Am I missing something obvious? Is this impossible without npm? Surely not.

@joshgoebel
Copy link
Member

joshgoebel commented Jun 13, 2020

Reading. I think my question is more about "Ambient Modules" though. Obviously adding 185 blah.ts.d files for each languages makes no sense, this seems to be why we have Ambient Modules and wildcards... I'm just not sure if they (their paths) only work with node_modules or if they work "in general" and I'm just not understanding the path dynamics yet.

https://www.typescriptlang.org/docs/handbook/modules.html#ambient-modules

@joshgoebel
Copy link
Member

IE, How does this work:

https://github.com/rtfpessoa/DefinitelyTyped/blob/919d0034b71f8652e793201f63d38f390bf343d7/types/highlight.js/index.d.ts#L161

declare module 'highlight.js/lib/highlight.js' {
	export = hljs;
}

Can it not be made to work with relative imports?

@joshgoebel
Copy link
Member

joshgoebel commented Jun 13, 2020

And if I futz around I can get this error, but googling didn't help much:

highlight.js/types/index.d.ts:217:16 - error TS2665: Invalid module name in augmentation. 
Module 'highlight.js/lib/core.js' resolves to an untyped module at 
'/Users/jgoebel/work/highlight.js/work/project/highlight.js/lib/core.js', which cannot be augmented.

@joshgoebel
Copy link
Member

Ok I gave up on the relative install and now I just installed it via npm (it's in package.json) but then just changed the node_mouldes/dir to a symlink to my build (for testing), and that also doesn't work for core.js or languages:

boot.ts:6:18 - error TS7016: Could not find a declaration file for module 'highlight.js/lib/core.js'. '/Users/jgoebel/work/highlight.js/build/lib/core.js' implicitly has an 'any' type.
  Try `npm install @types/highlight.js` if it exists or add a new declaration (.d.ts) file containing `declare module 'highlight.js/lib/core.js';`

6 import core from 'highlight.js/lib/core.js';
                   ~~~~~~~~~~~~~~~~~~~~~~~~~~

boot.ts:7:19 - error TS7016: Could not find a declaration file for module 'highlight.js/lib/languages/basic.js'. '/Users/jgoebel/work/highlight.js/build/lib/languages/basic.js' implicitly has an 'any' type.
  Try `npm install @types/highlight.js` if it exists or add a new declaration (.d.ts) file containing `declare module 'highlight.js/lib/languages/basic.js';`

7 import basic from 'highlight.js/lib/languages/basic.js';

@joshgoebel
Copy link
Member

joshgoebel commented Jun 13, 2020

So now I'm wondering if @types is some magic sauce that packages can't do on their own, but that seems entirely wrong - so I feel like I'm missing something simple... that should allow us to host the types (and Ambient modules) without our own repo and they "just work" when you install our package.

@joshgoebel
Copy link
Member

joshgoebel commented Jun 13, 2020

Ok I think for some reason npm was pointed to GitHub... but now that I have the literal NPM highlight.js package (the only thing I'm updating it copying in the index.d.ts file):

boot.ts:6:18 - error TS7016: Could not find a declaration file for module 'highlight.js/lib/core.js'. '/Users/jgoebel/work/highlight.js/work/project/node_modules/highlight.js/lib/core.js' implicitly has an 'any' type.
  Try `npm install @types/highlight.js` if it exists or add a new declaration (.d.ts) file containing `declare module 'highlight.js/lib/core.js';`

6 import core from 'highlight.js/lib/core.js';

node_modules/highlight.js/types/index.d.ts:214:16 - error TS2665: Invalid module name in augmentation. Module 'highlight.js/lib/core.js' resolves to an untyped module at '/Users/jgoebel/work/highlight.js/work/project/node_modules/highlight.js/lib/core.js', which cannot be augmented.

214 declare module 'highlight.js/lib/core.js' {

I don't understand this last error at all... I thought the whole point of augmentation like this was to add types to untyped JS files...

@nikeee
Copy link

nikeee commented Jun 13, 2020

I'm not sure if I get this problem right, so I try to answer some things mentioned in this issue.

So now I'm wondering if @types is some magic sauce that packages can't do on their own, but that seems entirely wrong - so I feel like I'm missing something simple... that should allow us to host the types (and Ambient modules) without our own repo and they "just work" when you install our package.

@types/* packages come exclusively via the DT repo. That way, we could adopt TS for libraries that did not want to opt-in to TS.
Currently, the @types/highlight.js is basically the same as in 2016:
https://github.com/DefinitelyTyped/DefinitelyTyped/commits/513510a89acc648422c9abb047265116198f99ee/highlightjs/highlightjs.d.ts
So they might be a bit outdated.

If you want to switch to a self-contained version that is installed with the normal js package, that's entirely possible (and even the preferred way of shipping types).
We then have to create a PR to DT stating that this happened. It basically must remove the existing types and add an entry to a file. This procedure is documented here:
https://github.com/DefinitelyTyped/DefinitelyTyped#removing-a-package

But I'm not sure what things should be possible with the new types or what use cases are not covered by the types in the @types/* package. Could you elaborate on that?

If it is planned for the future and it is open for contributions, I'd like to help with migrating hljs to TS.

@joshgoebel
Copy link
Member

joshgoebel commented Jun 13, 2020

That way, we could adopt TS for libraries that did not want to opt-in to TS.

Yes, I understand the purpose of @types.

If you want to switch to a self-contained version that is installed with the normal js package

I do, that's what I'm trying to do here - unsuccessfully. Before removing it from @types first I need to get it working with HLJS. My index.d.ts doesn't work, and copying over the exact one from @types also does not work.

So the version of the HLJS types I was looking at with the Ambient modules doesn't actually seem to be the official one... I was looking at someones fork. So this doesn't actually seem to work right now, even with the official package.

What I'm trying to do - allow deep requires into the library:

import core from 'highlight.js/lib/core.js';

Using code like (ambient modules):

declare module 'highlight.js/lib/core.js' {
    export = hljs;
}

I'm getting this error:

node_modules/@types/highlight.js/index.d.ts:162:16 - error TS2665: Invalid module name in augmentation. Module 'highlight.js/lib/core.js' resolves to an untyped module at '/Users/jgoebel/work/highlight.js/work/project/node_modules/highlight.js/lib/core.js', which cannot be augmented.

162 declare module 'highlight.js/lib/core.js' {

It seems pretty clear from that TS understands what I want to do, but I don't know why it "which cannot be augmented."

rodrigojcmello added a commit to rodrigojcmello/highlight.js that referenced this issue Jun 13, 2020
@joshgoebel
Copy link
Member

Can anyone weigh in on:

#2608

I think this resolves the issue and allows importing the library in all the supported variants. It seems to work in my quick testing.

@joshgoebel
Copy link
Member

Resolving with 10.1.1

@P4sca1
Copy link

P4sca1 commented Jun 17, 2020

@yyyc514 Thanks for the fix! I just migrated from @types/highlight.js to the types shipped with highlight.js v10.1.1.
It did not work at the beginning, imports to /lib/core or /lib/languages/lua failed with a module declaration not found error.
I had to add hightlight.js to my types array in tsconfig.json. I think this is needed because of the use of ambient modules.
Would be great if there was a little section in the docs called "TypeScript setup" where this is explained.

@joshgoebel
Copy link
Member

I had to add hightlight.js to my types array in tsconfig.json. I think this is needed because of the use of ambient modules.

Ambient modules work just fine in @types though... I'd be fine with adding docs (if that's needed) but first I'd like to understand the actual problem here and if it isn't something we can just fix in the code.

I thought that if I added a types key to package.json that this all should "just work".

/lib/core or /lib/languages/lua

How is our exact project setup? Code perhaps? You should be importing highlight.js/lib/core... I'm not sure hwo you are doing it without the name of the library first... I'd guess that might be part of the problem.

I tested all of these types of imports in a tiny TS project and they all worked:

import core from 'highlight.js/lib/core.js';
import hljs3 from 'highlight.js/lib/languages/basic.js';
import hljs from 'highlight.js';

@P4sca1
Copy link

P4sca1 commented Jun 17, 2020

I will try to provide a reproduction by tomorrow.

@nikeee
Copy link

nikeee commented Jun 17, 2020

Note that VSCode has a feature that pulls @types definitions for js libraries automatically in the background. This could lead to different behavior in VSCode and tsc respectively.
See: https://ticehurst.com/jsdocs/articles/types/ata.html

But I don't know whether this also affects pure TS projects. I just wanted to mention this because it may lead to issues.

@joshgoebel
Copy link
Member

Well it probably shouldn't do that if the library already has a types file though... I dunno how all this stuff works exactly.

@P4sca1
Copy link

P4sca1 commented Jun 18, 2020

It is explained in a little more detail here:
https://www.typescriptlang.org/docs/handbook/module-resolution.html#how-typescript-resolves-modules

I still don’t know why it works for you and why I have to add it to the types array.

@P4sca1
Copy link

P4sca1 commented Jun 18, 2020

@yyyc514 https://github.com/P4sca1/hljs-ts
Run yarn (or npm install) and then yarn ts-check or (npm run ts-check).
You will get the error unless you explictly add "types": ["highlight.js"] to "compilerOptions" in tsconfig.json.

@joshgoebel
Copy link
Member

I don't know the answer here someone from TS is goign to have to weigh in. It works if you start by loading the whole library:

import hljs from 'highlight.js';
import core from 'highlight.js/lib/core';
import hljs3 from 'highlight.js/lib/languages/basic';

Which is what I was seeing... but of course that's not a good test, oyu need to be able to load them individually, but that fails... yet I thought this exactly why @types existed and also that we should be able to do the same thing by bundling types with our library.

So I feel like we still just don't understand how we're supposed to do this yet but if someone could clue us it it should "just work".

@joshgoebel
Copy link
Member

joshgoebel commented Jun 18, 2020

I feel like you may need to add:

/// <reference types="highlight.js" />
import hljs from 'highlight.js/lib/core';

To your file if you aren't importing the ROOT of the library. This is also documented:

https://www.typescriptlang.org/docs/handbook/declaration-files/publishing.html

@nikeee
Copy link

nikeee commented Jun 19, 2020

I don't know the answer here someone from TS is goign to have to weigh in. It works if you start by loading the whole library:

import hljs from 'highlight.js';
import core from 'highlight.js/lib/core';
import hljs3 from 'highlight.js/lib/languages/basic';

This could break tree-shaking on some builds.

One way to do it without a /// ref would be to add a d.ts file for core. The PoC can be seen in this comment. The language definition don't have to get their own d.ts file.
It's just that the ambient modules for the language files have to be loaded. They don't get discovered automatically when highlight.js is not included.

I think if we'd use the solution mentioned in my comment, we can fix this. It's basically this:

mv index.d.ts core.d.ts
echo "import hljs from './core.js';\nexport = hljs;" > index.d.ts

If core is included, it has all types. If the highlight.js is included, it includes core (and the ambient module definitions in core).

@joshgoebel
Copy link
Member

joshgoebel commented Jun 19, 2020

I don't see any real solution here if it doesn't also solve for languages... Core is useless without also loading languages... am I missing something? And adding 180 + .d.ts files is ridiculous.

@nikeee
Copy link

nikeee commented Jun 19, 2020

And adding 180 + .d.ts files is ridiculous.

Just to be clear: This is not what I am proposing.

@joshgoebel
Copy link
Member

joshgoebel commented Jun 19, 2020

Sorry wasn't putting words in your mouth, just that seems to be the logical conclusion... Just fixing index and core is only 10% solving the problem here. I didn't see a solution for languages in your message.

I'm pretty sure the existing @types stuff doesn't handle core and languages either the way it currently stands... I really wish we had some TS experts to help us here on "best practices" vs "cute hacks", etc.

@joshgoebel
Copy link
Member

joshgoebel commented Jun 19, 2020

The language definition don't have to get their own d.ts file. It's just that the ambient modules for the language files have to be loaded.

Oh so maybe you did solve it and I just missed it hidden in the middle. Does this indeed solve everything because core has to be required to get any languages? This totally went over my head the first time - it's been a long week. :)

I'm not completely sure I follow "It's just that the ambient modules for the language files have to be loaded.".

declare module 'highlight.js/lib/languages/*' {
    export default function(hljs?: HLJSApi): LanguageDetail;
}

...seems to work now... I Presume it would also work in core, no?

@nikeee
Copy link

nikeee commented Jun 19, 2020

I've implemented my proposal based on the current master branch here:
master...nikeee:master

This is untested. Maybe it works for the above-mentioned case.

Edit:
It does work for that case (if the core.d.ts is moved to lib/), but not for the ambient modules of the language files.

Then /// refs could be an option. Even though I think 180+ files wouldn't be that crazy for this project. If this would be a TS project with --declaration, the compiler would emit these anyway.

Edit2:
It doesn't work because core.d.ts is a module. If it's not a module and the ambient module is in a separate file, it could work this way.

@joshgoebel
Copy link
Member

joshgoebel commented Jun 19, 2020

It does work for that case

I'm not such which "that case" is referring to...

It does work for that case (if the core.d.ts is moved to lib/), but not for the ambient modules of the language files.

So if you put it in lib you can't load core without issues but the languages still do not work? but the ambient modules were inside the core.d.ts, right? I'm not sure why that wouldn't work. I may have to try it myself.

Then /// refs could be an option. Even though I think 180+ files wouldn't be that crazy for this project. If this would be a TS project with --declaration, the compiler would emit these anyway.

I'm not sure I follow. I was suggesting that anyone USING our library would add the /// reference to their own source where they import it and done... annoying that it wouldn't be automatic though, but we could document it.

@joshgoebel
Copy link
Member

f it's not a module and the ambient module is in a separate file, it could work this way.

Yeah ambient modules don't seem to mix with top-level exports very well....

@nikeee
Copy link

nikeee commented Jun 19, 2020

The case: https://github.com/P4sca1/hljs-ts

So if you put it in lib you can't load core without issues but the languages still do not work?

Yes. It doesn't seem to work likely because core.d.ts is a module in my implementation (export = hljs).

If the language ambient module is in a separate file, it could work. But then we cannot use the types of hljs in them because they are not exported (only the hljs constant). Importing them also does not work.

Edit:
I now have it to the point where it compiles fine. But the default exports of the languages are not properly.

These files:

lib/core.d.ts:
Same as current highlight.js/master, but with /// <reference path="../types/languages.d.ts" /> at the top, export = hljs at the bottom and without declare modules

types/languages.d.ts:

declare module 'highlight.js/lib/languages/*' {
    export default function(hljs?: unknown): unknown;
}

types/index.ts:

import hljs from '../lib/core.js';
export = hljs;

With that setup, this has no errors:

import hljs from 'highlight.js/lib/core'
import basic from 'highlight.js/lib/languages/basic'

hljs.highlight("asd", "asd")

basic();

The parameter of basic is unknown because we cannot just import them in languages.d.ts.

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

No branches or pull requests

5 participants