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

fix "is not a module" #2603 #2606

Closed
wants to merge 2 commits into from

Conversation

rodrigojcmello
Copy link

No description provided.

@nikeee
Copy link

nikeee commented Jun 13, 2020

You can introduce support for highlight.js/lib/core.js like this:
https://github.com/highlightjs/highlight.js/compare/master...nikeee:ts?expand=1

Basically, you could do it for highlight.js/lib/highlight.js the same way.

However, I think it is not possible to do something for highlight.js/lib/languages/basic.js via ./languages/*.js.

Also, the types are not exported and available in TS (only the hljs object). It would require something different to also export the types.

IMHO, the easiest solution would be to migrate to TS and generate the declaration files via tsc. However, this would require some extra work to get the CJS version right.

@joshgoebel
Copy link
Member

joshgoebel commented Jun 13, 2020

I don’t want type files everywhere. Does the ambient module stuff simply not work at all? Why is it in the documentation? It looks like the perfect solution to this and I see lots of packages on @types that seem to be using it.

Example:

https://github.com/DefinitelyTyped/DefinitelyTyped/blob/12207c4a9dc09e7b486912c194674ff14989b612/types/store/index.d.ts

@nikeee
Copy link

nikeee commented Jun 13, 2020

You could try something like this (omitting the extension):

declare module 'lib/language/basic' {
    export = fn;
}

(When importing, the .ja extension is optional in TS). fn being a const of the languageFn.

Not tested.

@joshgoebel
Copy link
Member

I've been trying that, but nothing works so far. I guess today I'll try to find an NPM package and @types that actually seems to work correctly and then try to extrapolate from there.

@rodrigojcmello
Copy link
Author

I created a typing file for each submodule, and changed the types in the package.json to get the typing files out of the "types" folder.

I realized that there are two HLJSApi interfaces, I commented one because I didn't really understand the reason for having two. I need help with that.

It was also not very clear to me, what are the differences in the "highlight.js", "index.js" and "core.js" interface, I left them all the same. It would be good for someone to review this.

@joshgoebel
Copy link
Member

Can you please provide thoughts on #2608

This seems to work in my testing. And it supports not just pulling in the library itself, but also the variations people do most often.

@joshgoebel
Copy link
Member

Oh I'm just now seeing this, let me have a look.

@joshgoebel
Copy link
Member

I think ambient modules are much cleaner, no?

@joshgoebel
Copy link
Member

joshgoebel commented Jun 14, 2020

I realized that there are two HLJSApi interfaces, I commented one because I didn't really understand the reason for having two. I need help with that.

It's just extending the first, this is a feature of TS. I was trying to separate the mode helpers from the main API... though perhaps I should instead do this with a second interface and then extend it... or a & type...

It was also not very clear to me, what are the differences in the "highlight.js", "index.js" and "core.js" interface, I left them all the same. It would be good for someone to review this.

Well highlight.js and highlight.js/lib/index, are the same, so I removed the latter in my PR. core loads just the core library (no languages) where by default index pulls in EVERYTHING (core + load all languages, then export core).

@rodrigojcmello
Copy link
Author

rodrigojcmello commented Jun 14, 2020

I think ambient modules are much cleaner, no?

I did several tests, this was the only way I managed to make it work lol

I tried the way here #2608 but was unsuccessful.

@joshgoebel
Copy link
Member

I tried the way here #2608 but was unsuccessful.

Can you show me how you were unsuccessful or show me your test cases?

@joshgoebel
Copy link
Member

@rodrigojcmello How to structure the type file is a whole other discussion. :-) I'm sure it could be improved greatly and perhaps it should be more "type definition like". I'm not sure yet.

Right now I'm just first trying to solve the basic issue of importing our library in a TS project without blowing up. :)

@rodrigojcmello
Copy link
Author

Can you please provide thoughts on #2608

This seems to work in my testing. And it supports not just pulling in the library itself, but also the variations people do most often.

Here it is not clear whether you tested #2606 or #2608

I didn't get to test the #2608, yesterday I did something like that and it didn't work out. I'm going to test #2606 now.

@joshgoebel
Copy link
Member

joshgoebel commented Jun 14, 2020

I didn’t test 2606. I’ve very not interested in multiple files - if we can handle this instead with a single file and ambient modules. Which seems like they were designed for exactly this purpose

install("./types/types.ts","lib/types.ts");
install("./types/core.d.ts","lib/core.d.ts");
install("./types/highlight.d.ts","lib/highlight.d.ts");
install("./types/languages/basic.d.ts","lib/languages/basic.d.ts");
Copy link
Member

Choose a reason for hiding this comment

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

There are 180+ languages... this is why I'm not a fan of the individual file approach here.

@rodrigojcmello
Copy link
Author

Apparently the #2608 works very well.

My test yesterday probably failed because I declared the modules at the beginning of the file. Anyway, this solution is even better. 🥇

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.

None yet

3 participants