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

Remove 'del' package #33629

Merged
merged 3 commits into from Mar 6, 2019
Merged

Remove 'del' package #33629

merged 3 commits into from Mar 6, 2019

Conversation

j-oliveras
Copy link
Contributor

Starting on v4.0.0 it includes types declarations: https://github.com/sindresorhus/del/releases/tag/v4.0.0

Needs microsoft/types-publisher#586 to pass tests.

  • Use a meaningful title for the pull request. Include the name of the package modified.

  • Test the change in your own code. (Compile and run.)

  • Add or edit tests to reflect the change. (Run with npm test.)

  • Follow the advice from the readme.

  • Avoid common mistakes.

  • Run npm run lint package-name (or tsc if no tslint.json is present).

  • If a package was never on DefinitelyTyped, you don't need to do anything. (If you wrote a package and provided types, you don't need to register it with us.)

  • Delete the package's directory.

  • Add it to notNeededPackages.json.

@typescript-bot typescript-bot added this to Needs Author Attention in Pull Request Status Board Mar 5, 2019
@typescript-bot typescript-bot added Popular package This PR affects a popular package (as counted by NPM download counts). The Travis CI build failed labels Mar 5, 2019
@typescript-bot
Copy link
Contributor

typescript-bot commented Mar 5, 2019

@j-oliveras Thank you for submitting this PR!

🔔 @AyaMorisawa @BendingBender @bitjson @GiedriusGrabauskas @vladshcherbin @tkqubo - please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

If no reviewer appears after a week, a DefinitelyTyped maintainer will review the PR instead.

@typescript-bot
Copy link
Contributor

typescript-bot commented Mar 5, 2019

@j-oliveras The Travis CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

@j-oliveras
Copy link
Contributor Author

Only coping from opening post: needs microsoft/types-publisher#586 to pass tests.

@j-oliveras
Copy link
Contributor Author

j-oliveras commented Mar 5, 2019

Now, tests for gulp and vinyl-paths fails on TS 2.0 because that don't have Readonly. Is only used on tests, so a workaround is remove del from the tests.

rollup-plugin-delete fails on rollup declaration file (TS 2.3) with node_modules/rollup/dist/rollup.d.ts(376,24): error TS1110: Type expected.. The package is needed because the options extends from del. Is it caused because del own type not compatible with rollup-plugin-delete?

On rollup-plugin-delete seems to fails on rollup interface RollupOutput declaration:

export interface RollupOutput {
	output: [OutputChunk, ...(OutputChunk | OutputAsset)[]];
}

@sandersn what to do in this cases?

@sandersn
Copy link
Contributor

sandersn commented Mar 5, 2019

rollup-plugin-delete's error actually comes from rollup, which was already required. I'm not sure why it's failing now -- perhaps it was cached on the build server until now? In any case, rollup.d.ts used to have Typescript 2.3-compatible types before 1.2.0, but upgraded them at rollup/rollup#2679, which released on February 17.

So you have two choices:

  1. Make rollup-plugin-delete require TS 3.2.
  2. Make rollup-plugin-delete depend on rollup <1.2.0.

I recommend (2) since 3.2 is a pretty new version of Typescript still.

@sandersn
Copy link
Contributor

sandersn commented Mar 6, 2019

The reason rollup didn't fail until now is that the only time that we try to compile a package with all versions of typescript is when the package is actually changed. When it's just a dependent, it's only tested with typescript@next.

@j-oliveras
Copy link
Contributor Author

Done: changed gulp and vinyl-paths to not require del and make rollup on rollup-plugin-delete to < 1.2.0

@typescript-bot typescript-bot moved this from Needs Author Attention to Waiting for Reviewers in Pull Request Status Board Mar 6, 2019
@sandersn sandersn merged commit 03a9305 into DefinitelyTyped:master Mar 6, 2019
@j-oliveras j-oliveras deleted the remove-del-package branch March 6, 2019 18:43
@typescript-bot typescript-bot removed this from Waiting for Reviewers in Pull Request Status Board Mar 6, 2019
@typescript-bot
Copy link
Contributor

I just published @types/rollup-plugin-delete@0.2.1 to npm.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Popular package This PR affects a popular package (as counted by NPM download counts).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants