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

Bump parcel cli dependencies #5615

Merged
merged 21 commits into from Jan 20, 2021
Merged

Bump parcel cli dependencies #5615

merged 21 commits into from Jan 20, 2021

Conversation

aminya
Copy link
Contributor

@aminya aminya commented Jan 8, 2021

↪️ Pull Request

This PR bumps the dependencies for the cli package.

These dependency bumps include various performance, bug, and security fixes.

For example:

💻 Examples

🚨 Test instructions

CI and the tests pass.

✔️ PR Todo

  • Added/updated unit tests for this change
  • Filled out test instructions (In case there aren't any unit tests)
  • Included links to related issues/PRs

@height
Copy link

height bot commented Jan 8, 2021

Link Height tasks by mentioning a task ID in the pull request title or description, commit messages, or comments.

💡Tip: You can also use "Close T-X" to automatically close a task when the pull request is merged.

@aminya aminya changed the title ⬆️ Bump parcel cli dependencies Bump parcel cli dependencies Jan 8, 2021
@aminya
Copy link
Contributor Author

aminya commented Jan 8, 2021

Chalk has the types included in its package. Doesn't flow understand typescript types?

@mischnic
Copy link
Member

mischnic commented Jan 8, 2021

No, Flow doesn't understand Typescript definitons.
chalk used to also contain Flow definitions, but not anymore:
https://unpkg.com/browse/chalk@2.4.2/ vs https://unpkg.com/browse/chalk@4.1.0/

It the API hasn't changed (much), you could take https://unpkg.com/chalk@2.4.2/index.js.flow and add it to https://github.com/parcel-bundler/parcel/tree/v2/flow-libs (and wrapping the contents with declare module like in this file: https://github.com/parcel-bundler/parcel/blob/v2/flow-libs/ansi-html.js.flow)

@aminya
Copy link
Contributor Author

aminya commented Jan 8, 2021

@mischnic There is flowgen to automatically generate the flow definitions from .d.ts files.

@mischnic
Copy link
Member

mischnic commented Jan 8, 2021

There's no declare module, these are all global types and not scoped to the package.
And there's no // @flow comment at the top, so flow doesn't even read that file

@aminya
Copy link
Contributor Author

aminya commented Jan 8, 2021

There's no declare module, these are all global types and not scoped to the package.

I added this manually. This can be easily integrated into flowgen using an optional flag. I don't use flow anymore (converted everything using @khanacademy/flow-to-ts), so I don't have the time to send the PR.

And there's no // @flow comment at the top, so flow doesn't even read that file

There is a flag for this. --add-flow-header.

@aminya
Copy link
Contributor Author

aminya commented Jan 9, 2021

@mischnic Could you check why it still does not detect the types?

@mischnic
Copy link
Member

mischnic commented Jan 9, 2021

Renaming the file to chalk.js.flow worked, but apparently the types are still wrong because then I get

Cannot call `chalk.gray` because property `gray` is missing in  object type 

@aminya aminya force-pushed the cli-bump branch 2 times, most recently from feb08bf to 8fad7b6 Compare January 9, 2021 23:41
@aminya
Copy link
Contributor Author

aminya commented Jan 10, 2021

@mischnic I found a way to make flowgen output usable. You just have to remove the line which does npm$namespace and instead of that wrap the file in a declare module. You should be able to open a PR asking them to do this automatically.

Examples:
f33b731
e84638f
ac7b5d0

```
flowgen ./node_modules/term-size/index.d.ts -o 
./flow-libs/term-size.js.flow --add-flow-header --no-module-exports
```
```
flowgen ./node_modules/ora/index.d.ts -o ./flow-libs/ora.js.flow 
--add-flow-header --no-module-exports
```
@vidstige
Copy link

Who can review this? I'm getting security warnings just from installing latest from npm. 🙄

@aminya
Copy link
Contributor Author

aminya commented Jan 13, 2021

Who can review this? I'm getting security warnings just from installing latest from npm. 🙄

This is ready to go unless other PRs create conflicts.

@mischnic mischnic merged commit f7daae6 into parcel-bundler:v2 Jan 20, 2021
@aminya aminya deleted the cli-bump branch March 11, 2021 00:24
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