search

knapcio / react-leaflet-pixi-overlay

A react wrapper for the awesome Pixi Overlay: https://github.com/manubb/Leaflet.PixiOverlay
27 stars 16 forks source link

Package improvements and contribution #12

Closed firflant closed 3 years ago

firflant commented 3 years ago

Hi! I cloned the project and managed to run it locally. I noticed there is some space for improvements on the file structure in general:

1. react-dom depencency

react-dom is missing dev dependency. It results in a following error: Module not found: Error: Can't resolve 'react-dom' in '/Users/michal/Projects/react-leaflet-pixi-overlay/node_modules/react-leaflet/es'

2. Contribution and local setup guidelines The best way to test a clonned react-leaflet-pixi-overlay before commiting any change is to import it and use on a local react project. It can be done with help of yarn link functionality. All peer dependencies must be linked:

cd YOUR_CLONNED_REACT_LEAFLET_PIXI_OVERLAY_FORK
yarn link
yarn install
cd node_modules/react
yarn link
cd ../react-dom
yarn link
cd ../leaflet
yarn link
cd ../react-leaflet
yarn link

cd YOUR_REACT_PROJECT
yarn link react-leaflet-pixi-overlay
yarn link react
yarn link react-dom

This can be added to a readme file to make contribution easier for people.

3. Ignoring Build directory does not have to be included in a codebase. It can be added to .gitignore. The opposite for npm. When publishing a package, src directory and some other files unnecessarily extends the size of a npm package. It can be handled by adding new .npmignore file.

4. NPM vs Yarn There is both package-lock.json and yarn.lock file on a project. It's a good practice to use only one package manager. Otherwise it might cause issues with dependencies versioning when people contribute. It might be worth to consider removing one of lock files.

Please say what do you think about these improvements, then i can prepare a PR - i already applied some of these points locally :)

knapcio commented 3 years ago

Hi @frontcraft. Thanks for your analysis and the suggestions! All this seems very reasonable to me so please feel free to do the PR :)

firflant commented 3 years ago

PR is ready. I hope it is not too many at once!

knapcio commented 3 years ago

All good. Many thanks for the contribution!

I just merged your PR in, tested and published. Works like a charm, cheers :)