search

GeoTIFF / georaster-layer-for-leaflet

Display GeoTIFFs and soon other types of raster on your Leaflet Map
https://geotiff.github.io/georaster-layer-for-leaflet-example/
Apache License 2.0
306 stars 58 forks source link

Enhancement/59 implement typescript #60

Closed Rennzie closed 3 years ago

Rennzie commented 3 years ago

closes: https://github.com/GeoTIFF/georaster-layer-for-leaflet/issues/59

Opening PR for easier discussions.

Rennzie commented 3 years ago

Hey @DanielJDufour,

Sorry this has taken so long. In the interest of moving forward I've solved the last typing issues by silencing the compiler @ts-ignore. The code worked before so this won't stop it working correctly. This gives us a chance to test the repo and see what's needed before a full release.

As it stands there are three things outstanding:

  1. Get all the types output to dist/index.s.ts. I cannot figure out why this isn't happening but I don't think it's a blocker.
  2. Browserify: I've not set this up as I'm sure Webpack can do the job. I'd like to test this with a pre-release
  3. Comment tidy up: I'd like your feedback on the code before I clean the last bits out.

It would be great if you could have a look at and comment on the PR. If you've got the time to test it, that would be even better. Could I ask that you release to npm as a beta version. I'm writing a blog post on implementing COGs with a colleague and will be able to test it in a Code Sandbox. I'll also test loading from a <script/> tag which, if I have it right, is the use case for browserify?

I hope you're doing well and keeping safe.

Sean

DanielJDufour commented 3 years ago

@Rennzie , awesome. I'm busy through the weekend, but should have some time next week to do a look at it. Thank you!

Rennzie commented 3 years ago

Great stuff. Thanks @DanielJDufour

DanielJDufour commented 3 years ago

Update: Nevermind. I see your use of "files": [...] in the package.json.

I haven't used .npmignore before (from what I can remember). What files are included when you run npm pack? I just want to make sure we are excluding the test files and other un-needed files from the npm package.

DanielJDufour commented 3 years ago

I also learned a bunch about TS and other some cool tricks from looking at your code. Thank you!

Rennzie commented 3 years ago

Hey @DanielJDufour , sorry I've not gotten around to this yet. We've had some upheaval at work that needed attention. I'll hopefully have some time this week.

Rennzie commented 3 years ago

Hey @DanielJDufour , I'm sorry this has taken me so long. Hopefully it's in a place now thats workable. I've addressed all your comments, either with fixes in the code or further thoughts for discussion. Would love any further feedback you've got. The detailed reviews have been amazing to work with.

Rennzie commented 3 years ago

Hey @DanielJDufour thanks for reviewing so quickly. I'll set yInRasterPixels to 0 which will resolve the type error.

I'm new to beta releases too, but you can do npm publish --tag [alpha, beta, rc, next] which tells npm it's not the latest. Then npm install georaster-layer-for-leaflet will install the latest and not the tagged version. You'd have to explicitly do npm install georaster-layer-for-leaflet@alpha to get the tagged version. Reading the unpkg docs it respects this behaviour too.

Rennzie commented 3 years ago

Ok, all done and pushed.

DanielJDufour commented 3 years ago

Awesome. Would you be able to remove the "draft" from this PR, so I can merge it into master. Also, Github is showing that there is a merge conflict. Would you be able to resolve and then I can merge?

Screen Shot 2021-03-21 at 12 53 03 PM
Rennzie commented 3 years ago

@DanielJDufour conflicts all fixed and draft status removed.

DanielJDufour commented 3 years ago

@Rennzie , just merged. I just have a few small things to do before republishing. Thank you for all your awesome work!