johnagan / clean-webpack-plugin

A webpack plugin to remove your build folder(s) before building
MIT License
1.96k stars 137 forks source link

Move @types/webpack to dependencies rather than devDependencies #127

Open rdsedmundo opened 5 years ago

rdsedmundo commented 5 years ago
node_modules/clean-webpack-plugin/dist/clean-webpack-plugin.d.ts:1:33 - error TS7016: Could not find a declaration file for module 'webpack'. '/node_modules/webpack/lib/webpack.js' implicitly has an 'any' type.
  Try `npm install @types/webpack` if it exists or add a new declaration (.d.ts) file containing `declare module 'webpack';`

1 import { Compiler, Stats } from 'webpack';

I'm getting this error even though I'm not using typings on my Webpack configs at all, so it shouldn't be happening. I know I can just install the @types/webpack myself, but since the package is using it for something, it actually should be included as a dependency and installed automatically once I get the package.

From TS docs:

For that reason, we used "dependencies" and not "devDependencies", because otherwise our consumers would have needed to manually install those packages. If we had just written a command line application and not expected our package to be used as a library, we might have used devDependencies.

chrisblossom commented 5 years ago

The reason is because you should be installing @types/webpack in a typescript project with webpack.

Please provide a use case that you would install clean-webpack-plugin, but not webpack.

rdsedmundo commented 5 years ago

I'm not writing my webpack configs in TypeScript, so I don't see the reason why I should mandatorily add it as a dependency of my project.

chrisblossom commented 5 years ago

You most likely are using the allowJs and/or checkJs option, which you'd still need @types/webpack. That or set webpack to ignore your webpack config

chrisblossom commented 5 years ago

Moved @types/webpack to dependencies in https://github.com/johnagan/clean-webpack-plugin/pull/136.

Sebazzz commented 5 years ago

This change can actually break the build for web projects, as @types/node are now pulled into the scope causing jquery bindings to break among others.

chrisblossom commented 5 years ago

@Sebazzz please provide a repository showing this and/or link showing why/when this could happen.

Sebazzz commented 5 years ago

I don't have a minimal repo yet, but you can check out this project and update the clean-webpack-plugin package (yarn). The additional typings will now be taken into consideration for compilation.

rdsedmundo commented 5 years ago

I don't see how this will break any projects. Mine is also a Web project and it worked fine. Can you at least send the jQuery errors that were thrown?

chrisblossom commented 5 years ago

This is definitely causing a problem, but am unsure what to do to fix it.

@types/webpack-env fixes some of the problems.

I think we probably need to remove the @types/webpack dependency and then document that you might need to install it manually, as well as @types/webpack-env.

Thoughts?

rdsedmundo commented 5 years ago

What's the error that's happening?

If someone is experimenting an error because it's now installed automatically, they will keep facing it if you remove and then they install it manually (on a project that uses TypeScript).

It's better to understand and try to fix the error (if it's possible) rather than shallowing it.

chrisblossom commented 5 years ago

If someone is experimenting an error because it's now installed automatically, they will keep facing it if you remove and then they install it manually (on a project that uses TypeScript).

I agree, but I think it has to be solved on a project by project basis. The issue seems that @types/webpack brings in @types/node, which causes some issues, especially with front-end only applications.

It's better to understand and try to fix the error (if it's possible) rather than shallowing it.

Again, I agree. But I also don't think pulling in this library should cause additional typescript errors otherwise not present.

If anyone knows someone that is well-versed in Webpack with Typescript that we can get in this thread it would be great.

With the example provided by @Sebazzz:

Without @types/webpack-env:

js/App/App.ts:14:16 - error TS2339: Property 'hot' does not exist on type 'NodeModule'.

14     if (module.hot) {
                  ~~~

js/App/App.ts:15:16 - error TS2339: Property 'hot' does not exist on type 'NodeModule'.

15         module.hot.accept('./BindingHandlers/All', () => {
                  ~~~

js/App/Navigation.ts:8:16 - error TS2339: Property 'hot' does not exist on type 'NodeModule'.

8     if (module.hot) {
                 ~~~

js/App/Navigation.ts:9:16 - error TS2339: Property 'hot' does not exist on type 'NodeModule'.

9         module.hot.accept('App/Pages', () => {
                 ~~~

js/AppFramework/AppFactory.ts:223:16 - error TS2339: Property 'hot' does not exist on type 'NodeModule'.

223     if (module.hot) {
                   ~~~

js/AppFramework/AppFactory.ts:224:16 - error TS2339: Property 'hot' does not exist on type 'NodeModule'.

224         module.hot.accept('./BindingHandlers/All', () => {
                   ~~~

js/AppFramework/Components/LoadingBar.ts:171:31 - error TS2558: Expected 0 type arguments, but got 1.

171     public template = require<string>('./templates/loading-bar.html');
                                  ~~~~~~

js/AppFramework/Components/Modal.ts:256:20 - error TS2339: Property 'hot' does not exist on type 'NodeModule'.

256         if (module.hot) {
                       ~~~

js/AppFramework/Components/Modal.ts:257:20 - error TS2339: Property 'hot' does not exist on type 'NodeModule'.

257             module.hot.accept('./templates/modal.html', () => {
                       ~~~

js/AppFramework/Components/Popover.ts:58:20 - error TS2339: Property 'hot' does not exist on type 'NodeModule'.

58         if (module.hot && !this.template) {
                      ~~~

js/AppFramework/Components/Popover.ts:59:20 - error TS2339: Property 'hot' does not exist on type 'NodeModule'.

59             module.hot.accept('./templates/popover.html', () => {
                      ~~~

js/AppFramework/Forms/Confirmation.ts:64:12 - error TS2339: Property 'hot' does not exist on type 'NodeModule'.

64 if (module.hot) {
              ~~~

js/AppFramework/Forms/Confirmation.ts:65:12 - error TS2339: Property 'hot' does not exist on type 'NodeModule'.

65     module.hot.accept('./templates/confirmation.html');
              ~~~

With @types/webpack-env:

js/AppFramework/ServerApi/HttpClient.ts:78:28 - error TS2345: Argument of type 'jqXHR<any>' is not assignable to parameter of type 'Promise<T>'.
  Property 'finally' is missing in type 'jqXHR<any>' but required in type 'Promise<T>'.

78             requestHandler(promise);
                              ~~~~~~~

  node_modules/typescript/lib/lib.es2018.promise.d.ts:31:5
    31     finally(onfinally?: (() => void) | undefined | null): Promise<T>
           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    'finally' is declared here.

js/AppFramework/ServerApi/HttpClient.ts:81:9 - error TS2322: Type 'jqXHR<any>' is not assignable to type 'Promise<T>'.

81         return promise;
           ~~~~~~~~~~~~~~~

I am unsure how to solve the jQuery issues.

rdsedmundo commented 5 years ago

The problem here is at his code. He's using jQuery Promises but he's declaring them as natives Promises. It looks like their interfaces are not compatible anymore with the introduction of the finally, I suppose jQuery either doesn't support or that or its type definitions weren't updated if the support was added.

He should change those declarations here:

    public get<T>(url: string, data: any = null): Promise<T> {
        return this.method(url, 'GET', data);
    }

    public getText(url: string, data: any = null): Promise<string> {
        return this.method(url, 'GET', data, textRequest);
    }

    public put<T>(url: string, data: any = null): Promise<T> {
        return this.method(url, 'PUT', data);
    }

    public post<T>(url: string, data: any = null): Promise<T> {
        return this.method(url, 'POST', data);
    }

    public delete<T>(url: string, data: any = null): Promise<T> {
        return this.method(url, 'DELETE', data);
    }

to use JQuery. jqXHR<T> instead of Promise<T>.

Sebazzz commented 5 years ago

jQuery 3+ promises are actually compatible with ES promises, or at least, they were. You also may, for instance, await jquery promises.

rdsedmundo commented 5 years ago

See: https://github.com/jquery/jquery/issues/4290

So they've decided to not implement finally, therefore they are not compatible anymore with ES2018+. I'd suggest you commenting on this issue or opening another there with your use case.

chrisblossom commented 5 years ago

Thanks @rdsedmundo for tracking down the jquery issue!

This still leaves us with the @types/webpack-env issue. I think adding a note to the readme should be enough. What does everyone think?

Sebazzz commented 5 years ago

That seems like a logical solution.

pshurygin commented 5 years ago

This is definitely a breaking change, and a big one. I have just updated clean-webpack-plugin to version 3, which caused @types/node being installed into node_modules, which in turn caused million of typescript errors in our angular/typescript project, because now we had two type declarations for setTimeout and other browser methods.

The issue was really hard to investigate, as you wouldn't think that updating a plugin like this can cause these weird type errors across your project.

And the only workaround available is downgrading plugin version back to 2.x

chrisblossom commented 5 years ago

@pshurygin Have you installed @types/webpack-env? If so, what are the errors afterwards?

pshurygin commented 5 years ago

Just tried to install it, but it changes nothing. plugin v2 + @types/webpack-env = no errors plugin v3 + @types/webpack-env = errors

As it was mentioned in this thread, the issue is that @types/webpack, which is brought by v3, brings @types/node alongside self, which leads to errors in browsed-based typescript apps(conflicting type definitions between node and browser). Unless you remove it from dependencies, the issue would persist. Correct me if i'm wrong but i think that neither @types/webpack nor @types/webpack-env should be included in dependencies, because they are not required for the functioning of this plugin. I'm not sure why the OP had this issue, but this was obviously not the right solution for it.

To be clear, our webpack.config is native javascript, so we are not using @types/webpack or @types/webpack-env ourselves. Also we are not using allowJs option.

rdsedmundo commented 5 years ago

I'd say to revert this if it's causing too many problems.

I have the allowJs option turned on, although my webpack config is also on VanillaJS.

phil-lgr commented 4 years ago

the 'webpack' package has its own declaration files, '@types/webpack' should not be used anymore, this has been the case for months if not years

image

https://github.com/webpack/webpack/tree/master/declarations

please consider removing '@types/webpack' package and depending on 'webpack' only

phil-lgr commented 4 years ago

Here's an example of a plugin I built in TS without @types/webpack:

see https://github.com/noveo-io/puppeteer-prerender-webpack-plugin/blob/master/package.json#L64

and https://github.com/noveo-io/puppeteer-prerender-webpack-plugin/blob/master/lib/index.ts#L8

chrisblossom commented 4 years ago

I get the error Could not find a declaration file for module 'webpack'. without @types/webpack.

fwh1990 commented 3 years ago

Try to remove @types/webpack for webpack@5