Implement TypeScript - Githubissues

DanielJDufour commented 3 years ago

Is your feature request related to a problem? Please describe.

Developers using TypeScript would like more support (Intellisense, type-checking etc.)

Describe the solution you'd like

Create a georaster-layer-for-leaflet.ts file that is basically georaster-layer-for-leaflet.js but in typescript

Describe alternatives you've considered

using types file

Additional context

Please preserve the current build scripts, but basically add the ts-to-js conversion as a pre-processing step. This way users who want to import that unbuilt pure-JS file can continue to do so.
@Rennzie

Rennzie commented 3 years ago

@DanielJDufour does a georaster have a specific type signature?

I need it to type options.georasters and options.georaster properties.

DanielJDufour commented 3 years ago

Hi, @Rennzie . Unfortunately, I do not have one, but sounds like it would be a good idea to create one. Is that the same as the "typescript declaration file"?

Rennzie commented 3 years ago

Hey @DanielJDufour, I confused that more than need be 😂. So yes it would be put in a .d.ts declaration file but what I'm after is the type of each property in the georaster object. I assume that parseGeoraster function from your georaster library returns an object? It's the shape of that object I'm after.

i.e:

const georaster = {
  [property]: [type]
}

I'll try console log the result and start there.

Rennzie commented 3 years ago

Console logging a COG gives me:

{
   getValues: ƒ (e),
   height: 44032,
   noDataValue: null,
   numberOfRasters: 4,
   pixelHeight: 0.009330691929324869,
   pixelWidth: 0.009330691929288402,
   projection: 3857,
   rasterType: "geotiff",
   sourceType: "url",
   toCanvas: ƒ (e),
   width: 44544,
   xmax: -178258.31593243405,
   xmin: -178673.94227373227,
   ymax: 4497960.816012535,
   ymin: 4497549.966985503,
   _blob_is_available: true,
   _data: "https://storage.googleapis.com/hummingbird-share/cogs/lettuce_ms_cog.tif",
   _geotiff: Ce,
   bigTiff: false,
   cache: false,
   firstIFDOffset: 192,
   ghostValues: null,
   ifdRequests: (10) [Promise, Promise, Promise, Promise, Promise, Promise, Promise, Promise, Promise, Promise],
   littleEndian: true,
   source: ie {blockSize: 65536, blockRequests: Map(0), blocks: Map(37), blockIdsAwaitingRequest: null, retrievalFunction: ƒ},
   __proto__: xe,
   _url: "https://storage.googleapis.com/hummingbird-share/cogs/lettuce_ms_cog.tif",
   _url_is_available: true,
   _web_worker_is_available: true,
}

Which broadly speaking would make the type declaration look like this:


type Source = {
   blockSize: number, 
  // not sure how to type Map yet but thats easily solved once I know what the map is of. i.e number, string etc
   blockRequests: Map, 
   blocks: Map, 
   blockIdsAwaitingRequest: any | null, 
   retrievalFunction:() => void}

interface Georaster {
   getValues: (event: any) => void,
   height: number,
  // Likely more noDataValue options. Maybe using any is best
   noDataValue: null,
   numberOfRasters: number,
   pixelHeight: number,
   pixelWidth: number,
   projection: number,
  // this is probably an enum instead of string. Need the other types. 
   rasterType: "geotiff" | "jpeg",
  // Same here, probably enum
   sourceType: "url",
   toCanvas: (e: any) => void,
   width: number,
   xmax: number,
   xmin: number,
   ymax: number,
   ymin: number,
   _blob_is_available: boolean,
   _data: string,
   _geotiff: ??,
   bigTiff: boolean,
   cache: boolean,
   firstIFDOffset: number,
   ghostValues: null,
   ifdRequests: Promise[],
   littleEndian: boolean,
   source: Source,
   __proto__: ??,
   _url: string,
   _url_is_available: boolean,
   _web_worker_is_available: boolean,
}

The main question is, are those all the properties in the object parseGeoraster returns? Then of those properties which are options, which could return undefined or null and where I've put ?? I'm not sure what the type should be based on the console log.

Where we are unsure of type we can use any in the interim.

DanielJDufour commented 3 years ago

@Rennzie , thanks for pushing this forward. Here's some feedback:

rasterType could be "geotiff" or "object". I'm hoping to finish a new major release of georaster, which will include a lot of new raster types, but I'm not sure what those all are yet. We can update the interface when I know.
noDataValue could technically be anything, but at the moment there's only examples of it being undefined, null or a number, so it's safe to limit it to those three for the moment
sourceType could be 'Buffer', 'url', or undefined
_geotiff could be an object or undefined (I think the proto is on the geotiff object, but I don't think you need to define it. You could just use an object type without specifying its properties, I hope) let me know if there are any other questions.

Rennzie commented 3 years ago

Thanks @DanielJDufour, Thats big help. Some more questions for you:

In this piece of code:

     if (this.georasters.every((georaster: Georaster) => typeof georaster.values === "object")) {
        this.rasters = this.georasters.reduce((result: number[], georaster: Georaster) => {
          result = result.concat(georaster.values);
          return result;
        }, []);
        if (this.debugLevel > 1) console.log("this.rasters:", this.rasters);
      }

georaster references a values type. What would that look like? This did not show up in my console log tests above.

In the georaster object what do the getValues and toCanvas methods return if anything?
If I understand the code correctly, the argument options for getValues should all be numbers and cannot be undefined?

DanielJDufour commented 3 years ago

Good questions. 1) Values is a 3-dimensional array of numbers. In other words, an array of bands which are an array of rows which are an array of pixel values. It can also be undefined when the georaster is loaded from a URL. 2) Good questions.

getValues returns a 3-dimensional array of numbers. getValues may return something different than the values property if the user is passing in params to getValues that clip the box out of the larger raster space
toCanvas returns a canvas html element which is created here 3) the options object passed into getValues can be undefined as well as the properties inside of the options object. for example, if you don't specify the width, it should auto-calculate that from the left and right values.

DanielJDufour commented 3 years ago

thank you for helping with this. these questions really shine a light on how much more can be done to document and improve the DX. many thanks!

Rennzie commented 3 years ago

Hey @DanielJDufour,

Thanks for the answers. We moving it forward nicely. Hopefully this will make life easier for people to contribute 😄 .

I've run into a few blockers, mostly because I don't fully understand what some of the code needs to do. I'm going to open a PR with the WIP so we can discuss it more easily.

Rennzie commented 3 years ago

@DanielJDufour I'm looking at the build pipeline to see how best to sit tsc. What do you need/use browserify for? Can it ouput a file that webpack or babel can't?

Rennzie commented 3 years ago

Hey @DanielJDufour will you have a chance to look at my comments in the pr for this issue? I'm keen to get this work done this week if I can.

DanielJDufour commented 3 years ago

Hi, @Rennzie . We're mostly using browserify at this point for legacy reasons. Because some people are using it, I don't want to remove it. It's not something that absolutely couldn't be removed, but I'd strongly prefer to keep it in if possible.

Rennzie commented 3 years ago

@DanielJDufour one change I made was to add leaflet as a peerDependancy. This means I needed to tell webpack that it's an external. Webpack handles this fine in v5.0 but browserify doesn't seem to have a neat option built in. We would have to include another dependancy (browserify-shim) to resolve the external module and add some more config. I could take leaflet out but the plugin writing docs suggest including it in this way and the tsc compiler screamed at me without it 😂.

Could you give any more light on what you mean by "legacy reasons". I'm hoping the outputs from webpack will can resolve any issues.

What do you suggest? As you say the best solution would be to maintain browserify but I'm not sure the overhead it brings is worth it.

DanielJDufour commented 3 years ago

No problem. In the past, I've found it easier to load a library via browserify (with less errors and complications) when one is loading scripts via script tags (versus bundling an application via webpack, parcel, etc.). (I receive a lot of emails from folks who use GeoRasterLayer for Leaflet this way).

Browserify builds are often smaller than the webpack builds, which is true in our case, but not by much (7kb).

Does the browserify build work when you use the -external flag? What errors do you receive when you use -external? https://github.com/browserify/browserify#bexternalfile I found it via https://stackoverflow.com/questions/29222745/how-do-i-exclude-the-requirereact-from-my-browserified-bundle although it mentions some complications. Does the browserify-shim answer come from this StackOverflow thread?

DanielJDufour commented 3 years ago

@Rennzie , also, just wanted to say thank you for reaching out again. Sorry for the delay in response to your questions. I very much appreciate how much you are putting in to convert this lib to TypeScript. Not an easy feat!

Rennzie commented 3 years ago

Hey @DanielJDufour, no worries at all. I'm having fun and it's turning into a solid challenge 🎉. This is my first major contribution to OSS which means I'm learning loads about building a library. Thanks for baring with.

Re browserify: I got the *-shim suggestions from here. No one seems to have too much faith in it though 😩. I'd be curious to now if the UMD output by webpack still has the issues that you saw previously. Webpack 5.x has come a long way since the 3.x which was being used before. Another thing worth mentioning is that webpack wasn't minifying anything with the previous config. In the PR I've set it up to use the default minifier (Terser) so the .min.js output should be significantly smaller than the previous webpack output (not validated this myself yet). I'll keep digging in during the week.

DanielJDufour commented 3 years ago

@Rennzie , I just published version 1.0.0!

GeoTIFF / georaster-layer-for-leaflet

Implement TypeScript #59