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

Feature/typings #3

Merged
merged 43 commits into from
Mar 17, 2019
Merged

Conversation

SzybkiSasza
Copy link
Collaborator

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).

Copy link
Owner

@mattlewis92 mattlewis92 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great thanks! I think it might be better to just port the project itself across to TS so the typings never get out of date, WDYT?

@SzybkiSasza
Copy link
Collaborator Author

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
Copy link
Owner

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
Copy link
Collaborator Author

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

@SzybkiSasza
Copy link
Collaborator Author

@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 #2 as well.

@codecov-io
Copy link

codecov-io commented Feb 8, 2019

Codecov Report

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

Impacted file tree graph

@@            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
Copy link
Collaborator Author

@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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

@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 ;)

Copy link
Owner

@mattlewis92 mattlewis92 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, this is really awesome, thank you!! ❤️

The only comments I have are:

  • Let's keep things simple and just have one index.ts instead of multiple ones, I think this will simplify things a lot. If you add export default FilterWarningsPlugin to the index it won't break anything for people already using es modules in their webpack configs, and for commonjs users they can migrate to const { FilterWarningsPlugin } = require('webpack-filter-warnings-plugin')

  • Is having @types/webpack as a dependency a best practice? I'm just worried about when @types/webpack@5 is released and how npm will hoist things and if it could break typings by having duplicate module declarations. Are there any other webpack plugins out there that ship typings that we could copy from?

@SzybkiSasza
Copy link
Collaborator Author

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
Copy link
Collaborator Author

SzybkiSasza commented Mar 15, 2019

I updated the PR:

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 :)

Copy link
Owner

@mattlewis92 mattlewis92 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking awesome, thank you so much! Just a couple of small things and I think we're g2g

package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@SzybkiSasza
Copy link
Collaborator Author

SzybkiSasza commented Mar 16, 2019

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

@mattlewis92 mattlewis92 merged commit af1ad14 into mattlewis92:master Mar 17, 2019
@mattlewis92 mattlewis92 mentioned this pull request Mar 17, 2019
@mattlewis92
Copy link
Owner

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

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