Skip to content

fix: add @types dependencies for tapable + webpack#281

Merged
piotr-oles merged 3 commits intomasterfrom
beta
May 8, 2019
Merged

fix: add @types dependencies for tapable + webpack#281
piotr-oles merged 3 commits intomasterfrom
beta

Conversation

@johnnyreilly
Copy link
Copy Markdown
Member

@johnnyreilly johnnyreilly commented May 8, 2019

Fix this error when using strict package managers like pnpm/yarn pnp:

ERROR in /path/to/project/node_modules/.registry.npmjs.org/fork-ts-checker-webpack-plugin/1.2.0/node_modules/fork-ts-checker-webpack-plugin/lib/index.d.ts(1,26):
TS7016: Could not find a declaration file for module 'webpack'. '/path/to/project/node_modules/.registry.npmjs.org/webpack/4.30.0/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';`
ERROR in /path/to/project/node_modules/.registry.npmjs.org/fork-ts-checker-webpack-plugin/1.2.0/node_modules/fork-ts-checker-webpack-plugin/lib/index.d.ts(48,209):
TS7016: Could not find a declaration file for module 'tapable'. '/path/to/project/node_modules/.registry.npmjs.org/tapable/1.1.3/node_modules/tapable/lib/index.js' implicitly has an 'any' type.
  Try `npm install @types/tapable` if it exists or add a new declaration (.d.ts) file containing `declare module 'tapable';`
ERROR in /path/to/project/node_modules/.registry.npmjs.org/fork-ts-checker-webpack-plugin/1.2.0/node_modules/fork-ts-checker-webpack-plugin/lib/index.d.ts(48,253):
TS7016: Could not find a declaration file for module 'tapable'. '/path/to/project/node_modules/.registry.npmjs.org/tapable/1.1.3/node_modules/tapable/lib/index.js' implicitly has an 'any' type.
  Try `npm install @types/tapable` if it exists or add a new declaration (.d.ts) file containing `declare module 'tapable';`

@elliottsj @phryneas
fix: add @types dependencies for tapable + webpack
4ed5e63 
Fix this error when using strict package managers like pnpm/yarn pnp:

    ERROR in /path/to/project/node_modules/.registry.npmjs.org/fork-ts-checker-webpack-plugin/1.2.0/node_modules/fork-ts-checker-webpack-plugin/lib/index.d.ts(1,26):
    TS7016: Could not find a declaration file for module 'webpack'. '/path/to/project/node_modules/.registry.npmjs.org/webpack/4.30.0/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';`
    ERROR in /path/to/project/node_modules/.registry.npmjs.org/fork-ts-checker-webpack-plugin/1.2.0/node_modules/fork-ts-checker-webpack-plugin/lib/index.d.ts(48,209):
    TS7016: Could not find a declaration file for module 'tapable'. '/path/to/project/node_modules/.registry.npmjs.org/tapable/1.1.3/node_modules/tapable/lib/index.js' implicitly has an 'any' type.
      Try `npm install @types/tapable` if it exists or add a new declaration (.d.ts) file containing `declare module 'tapable';`
    ERROR in /path/to/project/node_modules/.registry.npmjs.org/fork-ts-checker-webpack-plugin/1.2.0/node_modules/fork-ts-checker-webpack-plugin/lib/index.d.ts(48,253):
    TS7016: Could not find a declaration file for module 'tapable'. '/path/to/project/node_modules/.registry.npmjs.org/tapable/1.1.3/node_modules/tapable/lib/index.js' implicitly has an 'any' type.
      Try `npm install @types/tapable` if it exists or add a new declaration (.d.ts) file containing `declare module 'tapable';`
@johnnyreilly johnnyreilly mentioned this pull request May 8, 2019
Merged
@johnnyreilly
Copy link
Copy Markdown
Member Author

johnnyreilly commented May 8, 2019 •
edited
Loading

Any idea why this says "This branch is out of sync with the base branch.
Changes from the base branch must be synced with this branch before merging."?

It looks like there's still something not quite right with our setup .... Any ideas @piotr-oles / @phryneas?

Are we safe to hit the "update" button or will that cause other problems?

@piotr-oles
Copy link
Copy Markdown
Collaborator

piotr-oles commented May 8, 2019

The only problem with "Update branch" will be that it will change the commit hash.
I can change a link in the "Realseses" manually to point to the proper commit.

There is one merge commit more on the master branch: e9ee9a from #275. I had to use a merge commit but that the last time it was used. Now merge commits are blocked and I believe that if we update this branch and merge with rebase it will work well 😀

piotr-oles and others added 2 commits May 8, 2019 08:55
@piotr-oles
Merge branch 'master' into beta
e3c3973
@piotr-oles
chore: fix travis deployment - revert to all_branches: true
c10f72e 
It's hard to test travis deployment on protected branches - that's why
we have to revert changes and use "all_branches: true" - specyfing more
than one branch won't work.
@johnnyreilly
Copy link
Copy Markdown
Member Author

johnnyreilly commented May 8, 2019

Now merge commits are blocked and I believe that if we update this branch and merge with rebase it will work well 😀

We'll get there! So by the sounds of it, after this we'll be at the point where semantic release works without extra attention? 🤞

@piotr-oles piotr-oles merged commit 3f66022 into master May 8, 2019
@piotr-oles
Copy link
Copy Markdown
Collaborator

piotr-oles commented May 8, 2019

🎉 This PR is included in version 1.3.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@vakrilov
Copy link
Copy Markdown

vakrilov commented May 10, 2019

This PR is causing problems with non-node projects.

@types/webpack has a dependency to @types/node. Having @types/webpack as a direct dependency of fork-ts-checker-webpack-plugin means that you will always get @types/node in your project (because of dependency chain).

It is OK for node project, but for non-node project you have all the node typings which might cause build time TS error because of conflicting types.

Would you consider moving the @types/webpack dependency as a devDependency - this way it won't be installed automatically.

@vakrilov vakrilov mentioned this pull request May 10, 2019
Merged
@johnnyreilly
Copy link
Copy Markdown
Member Author

johnnyreilly commented May 10, 2019

@arcanis - would this change suggested by @vakrilov work for yarn PnP?

@arcanis
Copy link
Copy Markdown
Contributor

arcanis commented May 10, 2019

I'm not sure it would since you depend on webpack in the index.d.ts file. TypeScript would emit errors if it couldn't be imported, right (even without considering PnP)?

@johnnyreilly
Copy link
Copy Markdown
Member Author

johnnyreilly commented May 10, 2019

You're right.... Hmmmm... Not sure what to suggest here. No @types/webpack, no yarn PnP support. But the alternative (current) situation is potential type conflicts.

I suppose this could be worked around in tsconfig.json with excludes or perhaps types

https://www.typescriptlang.org/docs/handbook/tsconfig-json.html

But that is not a great default experience... I'm not too sure what to suggest here. Any ideas gratefully received ...

@arcanis
Copy link
Copy Markdown
Contributor

arcanis commented May 10, 2019

Just to be sure - regular (non-PnP) installs would work without @types/webpack being available in the node_modules? How would TypeScript know that it's to be expected and not emit errors?

@phryneas
Copy link
Copy Markdown
Contributor

phryneas commented May 10, 2019 •
edited
Loading

It just doesn't check these, as these only describe the .js files we're shipping and most users don't write their own code against these, so the .d.ts are never evaluated.

I guess if we would any-type webpack.Compiler, our emmited types would not depend on webpack at all.

Otherwise, we could just copy over the definition of webpack.Compiler, webpack.Stats, typeof webpack.compilation.Compilation, tapable.AsyncSeriesHook and tapable.SyncHook (those five interfaces are all we use) into a .ts file of our own and just drop the dependencies to @types/webpack and @types/tapable completely, so that we don't accidentally add that dependency again in the future..

Or we just don't ship our own type definitions at all. (Or only a select subset, like types for the options.. although those weren't part of the types until recently). That way, there would be not dependency as well.

@phryneas
Copy link
Copy Markdown
Contributor

phryneas commented May 10, 2019 •
edited
Loading

Or we could just mark the ForkTsCheckerWebpackPlugin.getCompilerHooks & ForkTsCheckerWebpackPlugin.apply properties as @internal and activate compilerOptions.stripInternal for release builds.

That way, only these two properties (which depend on webpack) would be removed from the built types.

Edit: not a good idea. getCompilerHooks is a documented API, we shouldn't make that go away in the types.

phryneas added a commit to phryneas/fork-ts-checker-webpack-plugin that referenced this pull request May 10, 2019
@phryneas
fix: slightly loosen some types to get rid of prod dependency
385587d 
the built .d.ts will no longer depend on @types/webpack and
@types/tapable

✅ Closes: should resolve TypeStrong#281
@phryneas phryneas mentioned this pull request May 10, 2019
Merged
@phryneas
Copy link
Copy Markdown
Contributor

phryneas commented May 10, 2019

I tried some different things and I think for now the hotfix with minimal changes is over in #287. Please take a look if this works for everyone!

piotr-oles pushed a commit that referenced this pull request May 13, 2019
@phryneas @piotr-oles
fix: slightly loosen some types to get rid of prod dependency
5582911 
the built .d.ts will no longer depend on @types/webpack and
@types/tapable

✅ Closes: should resolve #281
@piotr-oles
Copy link
Copy Markdown
Collaborator

piotr-oles commented May 13, 2019

🎉 This issue has been resolved in version 1.3.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

@IlCallo IlCallo mentioned this pull request Oct 7, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@piotr-oles piotr-oles piotr-oles approved these changes

Assignees

No one assigned

Labels

released

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@johnnyreilly @piotr-oles @vakrilov @arcanis @phryneas @elliottsj