Odd import naming (on version 5 branch)

[Roaders]

Currently to get hold of the createLocal function I am having to do this:

import {v3} from "node-hue-api"

v3.v3.api.createLocal()

Or:

import hueApi from "node-hue-api"

hueApi.v3.v3.api.createLocal()

which is not great!

Much better would just be:

import {createLocal, Api} from "node-hue-api"

createLocal().connect().then((api: Api => {
    // do stuff
});

I recommend barrels to export all from each folder so everything is available from the plain import. Removes and deep paths into the package and makes moving things around in a refactor much easier as you don't break the external interface.

[Roaders]

It seems that quite a few types aren't exported either:

declare type LightStateType = model.LightState | KeyValueType;
declare type LightId = number | string | model.Light;
declare type LightsType = model.Light | model.Luminaire | model.Lightsource;

none of these are exported at all so I can't use them in my code when I want to use the same type. For example:

    public async hueApiSetLightState(id: string, state: LightStateType) {
        const api = await this.hueApi;
        api.lights.setLightState(id, state).then(undefined, (err) => {
            if (err)
                this.emit('warning', err.message);
        })
    };

[peter-murray]

Thanks for the information, appreciate it. I am still trying to wrap up a lot of the exports as I need to be backwards compatible whilst moving this across to TypeScript.

I don't necessarily want to export everything, as that will constrain me from making implementation changes in the future. This is still a first pass on the TypeScript conversion as I need to remove the any and missing definitions and I would expect the majority of these issue to drop out. I will hopefully get a new cut of this out by the end of the week.

[Roaders]

I would say any types exposed on a public function like the ones I show above should be exported to prevent issues like not being able to proxy a function call.

[peter-murray]

I have a new version 5.0.0-beta.2 in npm that you can try out. Still working on a few things before final cut release (mostly documentation related) but may change some more things after testing with TypeScript usage.

[mirkoschubert]

@peter-murray I also have some problems with importing it with ESM instead of CJS. Maybe I'm doing something wrong, but if I use something like that:

import { discovery } from 'node-hue-api'

...I get the message that your module is a Common JS module, but if I use this one:

import hue from 'node-hue-api'
const { discovery } = hue

...I get an error either, telling me that the discovery module isn't an ESM module at all:

node-hue-api/dist/esm/index.js:4
import * as discovery from './api/discovery';
^^^^^^
SyntaxError: Cannot use import statement outside a module

I'm using the latest beta 2 and can't use require for my project.

[peter-murray]

I have been dual building these releases now and they "seem" to have these issues flushed out in the later builds.

Can you try 5.0.0-beta.4 which was released today. Also can you please provide me with the version of Node.js that you are using?

[mirkoschubert]

@peter-murray I just updated to the beta 4 and tried both approaches again. Sadly I got the same errors:

First version:

import { discovery } from 'node-hue-api'
         ^^^^^^^^^
SyntaxError: Named export 'discovery' not found. The requested module 'node-hue-api' is a CommonJS module, which may not support all module.exports as named exports.
CommonJS modules can always be imported via the default export, for example using:

import pkg from 'node-hue-api';
const { discovery } = pkg;

    at ModuleJob._instantiate (internal/modules/esm/module_job.js:120:21)
    at async ModuleJob.run (internal/modules/esm/module_job.js:165:5)
    at async Loader.import (internal/modules/esm/loader.js:177:24)
    at async Object.loadESM (internal/process/esm_loader.js:68:5)

Second version:

import * as discovery from './api/discovery';
^^^^^^

SyntaxError: Cannot use import statement outside a module
    at wrapSafe (internal/modules/cjs/loader.js:1001:16)
    at Module._compile (internal/modules/cjs/loader.js:1049:27)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1114:10)
    at Module.load (internal/modules/cjs/loader.js:950:32)
    at Function.Module._load (internal/modules/cjs/loader.js:790:14)
    at ModuleWrap.<anonymous> (internal/modules/esm/translators.js:199:29)
    at ModuleJob.run (internal/modules/esm/module_job.js:169:25)
    at async Loader.import (internal/modules/esm/loader.js:177:24)
    at async Object.loadESM (internal/process/esm_loader.js:68:5)

I use NVM with the LTS version of node (14.17.3).

[peter-murray]

Hi @mirkoschubert,

I have looked a little further into this and managed to replicate something similar to what you report when I do not use the commonjs value for the module option in my Typescript configuration when consuming the library.

Are you building a Node.js project or a web based project with the library, also are sin plain JavaScript or TypeScript when this is happening?

If you have a bare bones example that you can share it would aid me in diagnosing the exact problem, but I am continuing the investigate this.

[lsphillips]

@peter-murray Seeing this issue with 5.0.0-beta.6, created a simple gist to demonstrate: https://gist.github.com/lsphillips/3eced7b41008b2b036c7af4e9a3f483b. This works when using 4.0.10; however with 5.0.0-beta.6 I see this error:

import * as discovery from './api/discovery';
^^^^^^

SyntaxError: Cannot use import statement outside a module
    at Object.compileFunction (node:vm:355:18)
    at wrapSafe (node:internal/modules/cjs/loader:1039:15)
    at Module._compile (node:internal/modules/cjs/loader:1073:27)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1138:10)
    at Module.load (node:internal/modules/cjs/loader:989:32)
    at Function.Module._load (node:internal/modules/cjs/loader:829:14)
    at ModuleWrap.<anonymous> (node:internal/modules/esm/translators:201:29)
    at ModuleJob.run (node:internal/modules/esm/module_job:175:25)
    at async Loader.import (node:internal/modules/esm/loader:178:24)
    at async Object.loadESM (node:internal/process/esm_loader:68:5)

In addition, I noticed the dist/esm/index.js file contains import statements that do not provide the file extension in the specifiers, in Node the file extension is mandatory.

[peter-murray]

@lsphillips Thank you for the Gist, it provided me with the ability to properly diagnose this.

It is a horrible manifestation of the CommonJS/ESM dual packaging mess that exists inside Node.js when you are trying to straddle the divide and provide a single code base that properly supports the two module systems.

I think I have this now resolved in 5.0.0-beta.7 would you care to try that now? One thing I did need to add was the --experimental-specifier-resolution=node on the ESM code for this to work when running the code in Node.js locally, but once provided the library was able to be require or import referenced.

[peter-murray]

The ESM code is generated from the TypeScript compiler, so I am still looking into whether or not there is a configuration option on the extension that I can utilize.

[mirkoschubert]

@peter-murray Sorry for the delay! I tried out 5.0.0-beta.7 and can now provide you with the code. First of all, the error message mentioned above is now gone with your latest update. But there's already another one:

node:internal/errors:464
    ErrorCaptureStackTrace(err);
    ^

Error [ERR_UNSUPPORTED_DIR_IMPORT]: Directory import '/Users/username/Projects/cli/hue-cli/node_modules/node-hue-api/dist/esm/api/discovery' is not supported resolving ES modules imported from /Users/username/Projects/cli/hue-cli/node_modules/node-hue-api/dist/esm/index.js
    at new NodeError (node:internal/errors:371:5)
    at finalizeResolution (node:internal/modules/esm/resolve:317:17)
    at moduleResolve (node:internal/modules/esm/resolve:756:10)
    at Loader.defaultResolve [as _resolve] (node:internal/modules/esm/resolve:867:11)
    at Loader.resolve (node:internal/modules/esm/loader:89:40)
    at Loader.getModuleJob (node:internal/modules/esm/loader:242:28)
    at ModuleWrap.<anonymous> (node:internal/modules/esm/module_job:76:40)
    at link (node:internal/modules/esm/module_job:75:36) {
  code: 'ERR_UNSUPPORTED_DIR_IMPORT',
  url: 'file:///Users/username/Projects/cli/hue-cli/node_modules/node-hue-api/dist/esm/api/discovery'

I'm relatively new to the ESM type importing scheme, so maybe the error is on my end. But you can test it out for yourself: https://github.com/mirkoschubert/hue-cli

In lib/register.cjs you can see my CJS version, in lib/register-esm.js is my ESM version of the same code. Both get the same error message.

I'm using the discovery module since there was a message at some point, that the v3.discovery module is deprecated. But I already tried it out in both ways and with the LTS and latest Node version with the same result. I'd appreciate any help :)

[peter-murray]

You need to add the --experimental-specifier-resolution=node to the Node.js command line to get around that one, as the module has directory references to the files, e.g. node lib/register-esm.js would need to become node --experimental-specifier-resolution=node lib/register-esm.js to eliminate that error.