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

Move @types/glob to devDependencies #332

Closed
wants to merge 1 commit into from
Closed

Conversation

Hinton
Copy link

@Hinton Hinton commented Oct 11, 2024

Type dependencies should be dev dependencies, that way they won't affect downstream packages.

This change seems to have no negative impacts on building or running tests in the project.

Resolves #330

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.

This is semantically incorrect and will also cause issues as we do use glob types in exported types

@Hinton
Copy link
Author

Hinton commented Oct 11, 2024

@MarshallOfSound Would moving it back to an optional dependency work? Currently we have to delete both types/gulp and types/minimatch on postinstall.

@aaclayton
Copy link

aaclayton commented Oct 24, 2024

This is causing big problems for us too. Our documentation builds (typedoc) are completely failing because of this unless we do clumsy manual workarounds.

Isn't glob now natively in typescript anyways? Why is an extra types package needed?

@erikian
Copy link
Member

erikian commented Oct 24, 2024

Isn't glob now natively in typescript anyways? Why is an extra types package needed?

It is, but at this moment @electron/asar supports Node 10.12.0 and up, while glob only became native TS on v9 which requires Node 16. We do have plans to upgrade all ecosystem packages to a newer Node version as soon as support for requireing ESM graphs makes it into Node LTS, but I'm afraid we probably won't be updating glob until then unless we need to land another breaking change in the meantime.

That being said, based on #267 / #268, looks like we've solved this in the past by inlining the types, and that got lost when @electron/asar was rewritten in TS in #320, so we can probably just re-land the changes from #268 – I'll open a PR for that in a minute.

@erikian erikian mentioned this pull request Oct 24, 2024
@erikian
Copy link
Member

erikian commented Oct 25, 2024

Fixed in #336

@erikian erikian closed this Oct 25, 2024
@Hinton Hinton deleted the glob-dev branch October 31, 2024 10:23
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.

Please remove dependency on @types/glob which is not required and causes breakage in typedoc builds
4 participants