SzybkiSasza commented 5 years ago

As Typescript gets more and more popular, adding typings to the repository would make importing package in Typescript environments much easier.

I tried to keep it simple and concise and affect as little as possible, the only external dependency being Webpack @types/webpack (not Webpack itself).

SzybkiSasza commented 5 years ago

The code isn't very complex and it's already written using ES6 classes - shouldn't be problematic, thanks to that ;)

The biggest challenge would be to properly build so it supports both Import/export syntax and commonJS.

I might try helping with that in my free time, but it'd take a bit to standardize and properly test.

mattlewis92 commented 5 years ago

The biggest challenge would be to properly build so it supports both Import/export syntax and commonJS.

You don't need to worry about that, ES6 modules are only useful for frontend where tree shaking is a thing, for node you just need to transpile to commonjs and it will work everywhere

SzybkiSasza commented 5 years ago

Perfect, I'll try to conjure sth in following days then ;)

SzybkiSasza commented 5 years ago

@mattlewis92 Basic work is done. All the tests are passing and all the code (including tests) is rewritten to Typescript, with a few improvements :)

I'll need another day to refine the tests (cover new type guards) and probably address https://github.com/mattlewis92/webpack-filter-warnings-plugin/pull/2 as well.

codecov-io commented 5 years ago

Codecov Report

Merging #3 into master will increase coverage by 4.16%. The diff coverage is 91.66%.

@@            Coverage Diff             @@
##           master       #3      +/-   ##
==========================================
+ Coverage    87.5%   91.66%   +4.16%     
==========================================
  Files           2        1       -1     
  Lines          16       24       +8     
  Branches        4        3       -1     
==========================================
+ Hits           14       22       +8     
+ Misses          2        1       -1     
- Partials        0        1       +1

Impacted Files	Coverage Δ
src/index.ts	`91.66% <91.66%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 81dac11...288746a. Read the comment docs.

SzybkiSasza commented 5 years ago

@mattlewis92 I've just finished coding! :) Tested the library with ES imports - it works fine.

Added function as an exclude option
Refined type checking for exclude
Modified package.json to support different TS checks
Updated security script to use npm audit

And a lot of smaller changes. Feel free to merge and if you don't mind - I'd be thankful if you can mention me as a contributor / typescript version author ;)

SzybkiSasza commented 5 years ago

One remark - I changed main to index.js which exports ES module. cjs is still available under dist/cjs. Do you think that's acceptable (would probably break for older users, hence - major version update would be needed)?

SzybkiSasza commented 5 years ago

@mattlewis92 another round of changes - I was able to update code structure to prepare two builds:

ES module - compatible that doesn't transpile exports
CJS module that works in any NodeJS environment

I also added some build tools and tests for them. I tested both ES and CJS and typings for TS. Importing is not touched after that last update - main exported module is CJS.

I also updated README. Please feel free to adjust it in any desirable way ;)

SzybkiSasza commented 5 years ago

Hi! I'm sorry for not responding for so long, but I was switching commercial projects and well... Didn't have any time for Open Source :)

For Webpack - I completely agree - we can safely assume that someone using this plugin will add Webpack typings as well. I'll also update modified README to inform about that in Typescript section.
About separate indexes - I remember I modelled it after one repo I found online (this solution guarantees proper separation between both), but I'm not so sure anymore - maybe I took one step too far. I'll experiment with it and update repo soon :)

SzybkiSasza commented 5 years ago

I updated the PR:

@types/webpack were moved to devDependencies and peerDependencies. That way, they won't be included in built files (as long as user is using NPM >= 3.x), but build itself will succeed.
As for ES modules / CommonJS - I experimented with different ways of exporting the plugin and if you agree about CommonJS exporting {WebpackFilterWarningsPlugin} as an object, then we're set. I'm against default exports, as they're considered harmful / problematic, according to at least a few sources: https://basarat.gitbooks.io/typescript/docs/tips/defaultIsBad.html https://blog.neufund.org/why-we-have-banned-default-exports-and-you-should-do-the-same-d51fdc2cf2ad

Note on default exports - export default is not enough to get class exported as a whole in CommonJS, it'll just add exports.default = WebpackFilterWarningsPlugin along exports = { FilterWebpackWarningsPlugin } in resulting file(so in reality - it'd only add default key to exported object, as commonJS exports would become ambiguous whereas in ES exports you can have both.

If you're OK with destructuring (that would be a breaking change), then we're set. If not, I can revert to this "double entry files" approach :)

SzybkiSasza commented 5 years ago

@mattlewis92 done! All the remarks are addressed, if it works for you, we should be good to go! 😀

mattlewis92 commented 5 years ago

Thanks again for your hard work on this! 😄

Just published a beta to npm, would you mind installing it in your project and double checking everything is ok?

npm i webpack-filter-warnings-plugin@next

mattlewis92 / webpack-filter-warnings-plugin

Feature/typings #3

Codecov Report