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: Move @types/glob to devDependencies where it belongs #267

Closed
wants to merge 1 commit into from

Conversation

cbartondock
Copy link

No description provided.

@cbartondock cbartondock requested a review from a team as a code owner April 7, 2023 16:44
@cbartondock cbartondock changed the title Move @types/glob to devDependencies where it belongs fix: Move @types/glob to devDependencies where it belongs Apr 7, 2023
@cbartondock
Copy link
Author

See #266

Copy link
Member

@MarshallOfSound MarshallOfSound left a comment

Choose a reason for hiding this comment

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

If anything this should be an actual dependency not an optional dependency. The index.d.td file directly references glob types. So this would break the TS build of most folks projects.

@MarshallOfSound
Copy link
Member

That is, unless the version of glob we depend on has unlined types. In which case, we should remove this dependency completely.

Updating glob to a version with inline types is also acceptable if it doesn't result in a breaking change

@cbartondock
Copy link
Author

Hmmm. I think glob only added full typescript support in 9.0, which most definitely has breaking changes: https://github.com/isaacs/node-glob/blob/main/changelog.md

In the end the solution is to upgrade the glob dependency anyway and make sure that those breaking changes don't make it downstream to the asar api.

Should I try to do that or would you rather leave it? I fixed my issue by just removing the optional dependency on @types/glob from package-lock.json.

@isaacs
Copy link
Contributor

isaacs commented Apr 9, 2023

I'd highly recommend upgrading to glob v9 if possible. Yes, there are breaking changes to the API, but unless you're re-exporting the API directly, that shouldn't matter. The other hazard may be node version compatibility, I suppose.

@MarshallOfSound
Copy link
Member

unless you're re-exporting the API directly, that shouldn't matter

Let me tell you a sad story... 😢

I think the short-term fix here is to somehow inline the type, or just fallback the type to being any. Not many people even use that part of the asar API anyway but I assume as more and more people pick up glob v9 the types issue will become more prevalent.

The type resolution issue itself though seems like an annoying edge case between package managers wanting to hoist things and then also not contextually understanding that hoisting @types/foo and foo to the same level is gonna cause issues.

@isaacs
Copy link
Contributor

isaacs commented Apr 9, 2023

Ah, yes, @electron/asar supports node versions back to 10.12. So, unless you're willing to update that, the new glob won't work. (Imo, you should; node v10 has been unsupported for years now, even v14 is past its LTS end of life.)

That being said, there's still a puzzle here, because of the somewhat annoying way that DT types hijack the types defined by a module. So someone using glob for some other purpose will have their build break, because @types/glob will get placed higher up in the tree and cause anything else using the newer version of glob to pick up the old types.

If you do insist on maintaining support for node v10 (a noble goal, even if it's not one I'd ever want to take on), you can avoid the conflict by listing @types/glob in bundleDependencies, so that it's published in the @electron/asar, and excluded from npm's tree optimizations, or even put the @types/glob and @types/minimatch types in ./lib, or inline the IGlobOptions and IMinimatchOptions types into your index.d.ts file.

@isaacs
Copy link
Contributor

isaacs commented Apr 9, 2023

Inlining the types is really not that bad, since it's just the options interfaces, and I can promise you that the types for glob v7 will never change 😂

@MarshallOfSound
Copy link
Member

Superceded by #268

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