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

Add Typings for Node Platform #766

Merged
merged 8 commits into from Feb 16, 2023
Merged

Add Typings for Node Platform #766

merged 8 commits into from Feb 16, 2023

Conversation

KiwiKilian
Copy link
Contributor

@KiwiKilian KiwiKilian commented Feb 3, 2023

I augmented the module as far as I could figure out the types from the docs and usage. As I see there are a few more (hidden?) functions from the native module. I'm not sure if these should also be augmented and if so how:

  setBackendType: [Function: setBackendType],

  ParseError: [class ParseError extends Error],

  Expression: [Function: Expression] { parse: [Function: parse] },

Retrieved via console.log(mbgl). Also I'm not sure how to extend the module with events.EventEmitter.

@KiwiKilian KiwiKilian changed the title Add typings for Node Platform Add Typings for Node Platform Feb 3, 2023
@KiwiKilian
Copy link
Contributor Author

KiwiKilian commented Feb 3, 2023

For documentation purposes, I'm successfully using this augmentation like this:

function render({ center, zoom, width, height }: { center: [number, number]; zoom: number; width: number; height: number }) {
    return new Promise((resolve, reject) => {
      const options: mbgl.MapOptions = {
        request(req, callback) {
          request(
            {
              url: req.url,
              encoding: null,
              gzip: true,
            },
            (error, response, body) => {
              if (error) {
                callback(error);
              } else if (response.statusCode === 200) {
                const result: mbgl.RequestResponse = { data: body, modified: undefined, expires: undefined };

                if (typeof response.headers.modified === 'string') {
                  result.modified = new Date(response.headers.modified);
                }
                if (response.headers.expires) {
                  result.expires = new Date(response.headers.expires);
                }
                if (response.headers.etag) {
                  result.etag = response.headers.etag;
                }

                callback(undefined, result);
              } else if (response.statusCode === 404) {
                // Some tiles might be missing...
                callback();
              } else {
                callback(new Error(JSON.parse(body).message));
              }
            },
          );
        },
      };

      const map = new mbgl.Map(options);
      map.load(style);

      map.render(
        {
          width,
          height,
          zoom,
          center,
        },
        (err, buffer) => {
          if (err) {
            reject(err);
          }

          map.release();

          const image = sharp(buffer, {
            raw: {
              width,
              height,
              channels: 4,
            },
          });

          resolve(image.toFormat('jpg').toBuffer());
        },
      );
    });
  }

@louwers louwers added the node label Feb 3, 2023
@ovivoda
Copy link
Contributor

ovivoda commented Feb 9, 2023

@KiwiKilian can you please add a bit more context to the change? Why it is needed and how went wrong before?

@KiwiKilian
Copy link
Contributor Author

KiwiKilian commented Feb 9, 2023

@ovivoda Those typings improve the usage of the Node package in combintation with TypeScript or IDEs capable of supplying hints from JSDoc etc. It's basically a programmatic documentation of the API and their parameters.

Other Maplibre packages also supply those typings (e.g. Maplibre GL JS, maplibre/maplibre-gl-js#32). I would consider it best practice, to ship TypeScript typings with the package, for this the types field in package.json is used.

platform/node/index.d.ts Outdated Show resolved Hide resolved
@HarelM
Copy link
Member

HarelM commented Feb 9, 2023

I'm not super familiar with the project, but can you make sure this new file will be picked up when using npm publish?

@KiwiKilian
Copy link
Contributor Author

I'm not super familiar with the project, but can you make sure this new file will be picked up when using npm publish?

Thanks for pointing out, I thought the types field would be enough to publish. I added the index.d.ts to files, now it should be published:

npm publish --dry-run
npm notice 
npm notice 📦  @maplibre/maplibre-gl-native@5.1.1
npm notice === Tarball Contents === 
npm notice 30.2kB LICENSE.mbgl-core.md    
npm notice 1.4kB  LICENSE.md              
npm notice 4.0kB  README.md               
npm notice 2.2kB  package.json            
npm notice 1.2kB  platform/node/index.d.ts
npm notice 1.8kB  platform/node/index.js  
npm notice 7.0kB  platform/node/README.md 

@HarelM
Copy link
Member

HarelM commented Feb 9, 2023

LGTM in terms of syntax. Thanks!

@acalcutt
Copy link
Collaborator

@KiwiKilian is there anything else you think this needs or do you feel this is ready? I'm not sure we had a good answer for you on the one unresolved conversation on callback.

@KiwiKilian
Copy link
Contributor Author

@acalcutt I think this is a good to go. The events listening is not typed yet, but I think this could be added at any later point. This lays out the base functionality to make the TypeScript dev experience much better.

@acalcutt acalcutt merged commit 6b80162 into maplibre:main Feb 16, 2023
@etnav
Copy link
Contributor

etnav commented Mar 1, 2023

Just checked the usage of the typings on my end. Now only getting render, release and load, what about the rest, such as the ones below?

        static void New(const Nan::FunctionCallbackInfo<v8::Value>&);
        static void Loaded(const Nan::FunctionCallbackInfo<v8::Value>&);
        static void Cancel(const Nan::FunctionCallbackInfo<v8::Value>&);
        static void AddSource(const Nan::FunctionCallbackInfo<v8::Value>&);
        static void RemoveSource(const Nan::FunctionCallbackInfo<v8::Value>&);
        static void AddLayer(const Nan::FunctionCallbackInfo<v8::Value>&);
        static void RemoveLayer(const Nan::FunctionCallbackInfo<v8::Value>&);
        static void AddImage(const Nan::FunctionCallbackInfo<v8::Value>&);
        static void RemoveImage(const Nan::FunctionCallbackInfo<v8::Value>&);
        static void SetLayerZoomRange(const Nan::FunctionCallbackInfo<v8::Value>&);
        static void SetLayerProperty(const Nan::FunctionCallbackInfo<v8::Value>&);
        static void SetFilter(const Nan::FunctionCallbackInfo<v8::Value>&);
        static void SetCenter(const Nan::FunctionCallbackInfo<v8::Value>&);
        static void SetZoom(const Nan::FunctionCallbackInfo<v8::Value>&);
        static void SetBearing(const Nan::FunctionCallbackInfo<v8::Value>&);
        static void SetPitch(const Nan::FunctionCallbackInfo<v8::Value>&);
        static void SetLight(const Nan::FunctionCallbackInfo<v8::Value>&);
        static void SetAxonometric(const Nan::FunctionCallbackInfo<v8::Value>&);
        static void SetXSkew(const Nan::FunctionCallbackInfo<v8::Value>&);
        static void SetYSkew(const Nan::FunctionCallbackInfo<v8::Value>&);

Is something along the lines of:

setLayoutProperty: (layer: any, propertyName: string, propertyValue: string) => void;

correct?

@KiwiKilian
Copy link
Contributor Author

I don't know anything about those functions, but if they are available through the node binding, feel free to open another PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants