Please add {"type": "module"} to package.json

laverdet commented 2 years ago

You are publishing a module but not declaring it as such in package.json which makes it impossible to use

-> % cat test.mjs 
import {} from "ip-address";

-> % node test.mjs
(node:71803) Warning: To load an ES module, set "type": "module" in the package.json or use the .mjs extension.
(Use `node --trace-warnings ...` to show where the warning was created)
./node_modules/ip-address/dist/esm/ip-address.js:1
import { Address4 } from './lib/ipv4';
^^^^^^

SyntaxError: Cannot use import statement outside a module
    at Object.compileFunction (node:vm:352:18)
    at wrapSafe (node:internal/modules/cjs/loader:1033:15)
    at Module._compile (node:internal/modules/cjs/loader:1069:27)
    at Module._extensions..js (node:internal/modules/cjs/loader:1159:10)
    at Module.load (node:internal/modules/cjs/loader:981:32)
    at Module._load (node:internal/modules/cjs/loader:827:12)
    at ModuleWrap.<anonymous> (node:internal/modules/esm/translators:170:29)
    at ModuleJob.run (node:internal/modules/esm/module_job:198:25)
    at async Promise.all (index 0)
    at async ESMLoader.import (node:internal/modules/esm/loader:409:24)

Node.js v18.1.0

Currently to use this module we have to explicitly import as a CommonJS module:

import { createRequire } from "module";
const { Address4, Address6 } = createRequire(import.meta.url)("ip-address") as typeof import("ip-address");

maxpain commented 2 years ago

The same problem

maxpain commented 2 years ago

@bardiharborow could you please look into this?

beaugunderson commented 2 years ago

this was supposed to do this for our hybrid approach:

https://github.com/beaugunderson/ip-address/blob/master/.fix-package-types.sh

laverdet commented 2 years ago

@beaugunderson

this was supposed to do this for our hybrid approach:

https://github.com/beaugunderson/ip-address/blob/master/.fix-package-types.sh

Yeah idk, these aren't in the published version:

-> % npm install ip-address
added 3 packages, and audited 4 packages in 621ms
found 0 vulnerabilities

-> % find node_modules/ip-address -name package.json
node_modules/ip-address/package.json

Anyway, best practice is still to put "type": "module" in the top-level package.json, and to publish only one package.json. This is recommended in the nodejs manual.

I just gave this a spin locally and there is an additional problem-- missing file extensions. These are required for es modules, and optional for CommonJS modules. I know this is annoying and abrupt change in coding style but the ecosystem has moved in this direction and there is nothing we can do about it.

Happy to submit a PR with all the discussed changes [file extensions, remove nested package.json, add "type": "module"]. There's no telling if this will break anyone's workflow, but if does then their workflow is wrong.

beaugunderson commented 2 years ago

seems like a lot has changed anyway since those hybrid ESM changes went in—I'm traveling but I see the PR you opened, thank you! really just want to understand what changes for non-ESM users if we merge that…

On Mon, Jul 04, 2022 at 4:01 AM, Marcel Laverdet @.***> wrote:

@beaugunderson https://github.com/beaugunderson

this was supposed to do this for our hybrid approach:

https://github.com/beaugunderson/ip-address/blob/master/. fix-package-types.sh

Yeah idk, these aren't in the published version:

-> % npm install ip-address added 3 packages, and audited 4 packages in 621ms found 0 vulnerabilities

-> % find node_modules/ip-address -name package.json node_modules/ip-address/package.json

Anyway, best practice is still to put "type": "module" in the top-level package.json, and to publish only one package.json. This is recommended in the nodejs manual https://nodejs.org/api/packages.html#dual-commonjses-module-packages.

I just gave this a spin locally and there is an additional problem-- missing file extensions. These are required for es modules https://nodejs.org/api/packages.html#extensions-in-subpaths, and optional for CommonJS modules. I know this is annoying and abrupt change in coding style but the ecosystem has moved in this direction and there is nothing we can do about it.

Happy to submit a PR with all the discussed changes [file extensions, remove nested package.json, add "type": "module"]. There's no telling if this will break anyone's workflow, but if does then their workflow is wrong.

— Reply to this email directly, view it on GitHub https://github.com/beaugunderson/ip-address/issues/153#issuecomment-1173616014, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAPCX2LSLKTG4PHP6BXZ4DVSKY65ANCNFSM5WQZNCPA . You are receiving this because you were mentioned.Message ID: @.***>

laverdet commented 2 years ago

I tested the following in node versions v10 - v18:

test.mjs

import {} from "ip-address";
import { createRequire } from "module";
const { Address4, Address6 } = createRequire(import.meta.url)("ip-address");

test.js

require('ip-address');

--in additional to npm run test. I didn't, for example, test esbuild, Webpack, Rollup, etc which could have different results, but I would doubt it.

The second commit brings the published package closer in line to what you're already publishing. The nested package.json files weren't published before, because of the package.json files field [you can verify with npm publish --dry-run]. So now they will be published which is what could result in different results in exotic package managers. Again, I don't think this will be a problem.

laverdet commented 2 years ago

I have the updated module published to npm as @laverdet/beaugunderson-ip-address. Our CI/CD conventions forbids git references so I put up there instead.

maxpain commented 2 years ago

Any updates on this?

maxpain commented 2 years ago

Up

bennettp123 commented 1 year ago

☝🏻@beaugunderson I've thrown together a PR that sets type: module in the top-level package.json.

tests are passing on all node versions defined in .travis.yml (plus a couple new ones)
code coverage is working too
includes dependency fixes for node 18

Please let me know if there's anything else I can do to help get it merged/released! 😄

Towerism commented 1 year ago

My team switched to this package https://github.com/sindresorhus/is-ip

thib3113 commented 1 year ago

Hello @laverdet because you already forked it and publish it (but don't open issues on your repo)

Maybe can you correct the typing error : https://github.com/beaugunderson/ip-address/issues/143 . So I can use your fork without creating a new one :) .

Abdull commented 1 year ago

(using ip-address version 8.1.0)

Quoting OP:

-> % cat test.mjs 
import {} from "ip-address";

-> % node test.mjs
...

... Currently to use this module we have to explicitly import as a CommonJS module:

import { createRequire } from "module";
const { Address4, Address6 } = createRequire(import.meta.url)("ip-address") as typeof import("ip-address");

For anyone trying OP's workaround in an .mjs / ECMAScript file, then scratching their head over the following error message:

const { Address4, Address6 } = createRequire(import.meta.url)("ip-address") as typeof import("ip-address");
                                                                            ^^

SyntaxError: Unexpected identifier
    at ESMLoader.moduleStrategy (node:internal/modules/esm/translators:119:18)
    at ESMLoader.moduleProvider (node:internal/modules/esm/loader:468:14)
    at async link (node:internal/modules/esm/module_job:68:21)

Node.js v18.17.1

That is because OP's workaround is for TypeScript.

Here is a workaround for .mjs / ECMAScript files:

import { createRequire } from 'module';
const { Address4, Address6 } = createRequire(import.meta.url)('ip-address');

.mjs / ECMAScript workaround example:

File import-workaround-ip-address.mjs:

#!/usr/bin/env node
'use strict';

import { createRequire } from 'module';
const { Address4, Address6 } = createRequire(import.meta.url)('ip-address');

const someAddress6 = new Address6('2001:0:ce49:7601:e866:efff:62c3:fffe');
var someAddress6Teredo = someAddress6.inspectTeredo();

console.log(someAddress6Teredo.client4); // '157.60.0.1'

File package.json:

{
  "name": "npm-ip-address",
  "version": "1.0.0",
  "description": "",
  "main": "import-workaround-ip-address.mjs",
  "scripts": {
    "test": "echo \"Error: no test specified\" && exit 1"
  },
  "author": "",
  "license": "MIT",
  "dependencies": {
    "ip-address": "^8.1.0"
  }
}

Execution:

node import-workaround-ip-address.mjs

# or
node .

# or, with execute permissions set (`chmod +x import-workaround-ip-address.mjs`):
./import-workaround-ip-address.mjs

laverdet commented 1 year ago

Off topic but you should totally use TypeScript. You're coding with one hand behind your back my friend.

Edit: Why are you booing me? I'm right!

beaugunderson commented 1 year ago

Does anyone have a pointer to THE guide for packaging a TypeScript module to support all consumers, whether TS/require/mjs? I would really like to fix this for good. :)

thib3113 commented 1 year ago

@beaugunderson I never really found a good guide :/ .

But I think, I get something that works : https://github.com/thib3113/node-crowdsec/blob/main/packages/crowdsec-client/package.json

package.json type = module
bundle with esbuild => that will output an ESM/CJS/typing ( ESM will import only ESM when bundled, so the size is not really important )
exports and other in the package.json : https://github.com/thib3113/node-crowdsec/blob/main/packages/crowdsec-client/package.json#L5-L39 ( default need to be the last ... some tools will stop on it if not the last one )
just created some root files to link to the correct one in folders (automatically with esbuild config)

you can see the builded version : https://www.npmjs.com/package/crowdsec-client?activeTab=code .

And so, it will works with ESM/CJS/Typescript .

CJS will use the package.json main in most of cases ( so end to ./lib/index.cjs => ./lib/cjs/index.cjs )

ESM will follow the exports : import from "ip-address" => exports.['.'].import.default ( also, ESM will do three shaking, so, when bundled, will not include the cjs code )

typescript will follow types or the exports ( not really sure how it handle this, and when it follow one or the other )

laverdet commented 1 year ago

Does anyone have a pointer to THE guide for packaging a TypeScript module to support all consumers, whether TS/require/mjs? I would really like to fix this for good. :)

Honestly, just drop CommonJS. For users running a bundler (Webpack, Rollup, Vite, esbuild, whatever) they can still use require.

Prolific npm micropackage author Sindre Sorhus dropped CommonJS in 2021: https://twitter.com/sindresorhus/status/1349300123151450114

node-fetch: Dropped CommonJS in 3.x: https://github.com/node-fetch/node-fetch/commit/b50fbc105755123cad34fb0bc9d5653ecc693b8a

deno: CommonJS is hurting JavaScript https://deno.com/blog/commonjs-is-hurting-javascript

My opinion on this is that continuing to publish CommonJS at all is unethical since you are creating more work for yourself which could be better spent elsewhere.

beaugunderson commented 1 year ago

My opinion on this is that continuing to publish CommonJS at all is unethical since you are creating more work for yourself which could be better spent elsewhere.

this makes a lot of assumptions :)

I am not immune to the argument that CommonJS is past its useful date but there are still people who are stuck in legacy hell and want e.g. security updates or people who want to play in a node REPL without e.g. const { default: got } = await import("got");.

the deno article is great for this gem:

I'll look at dnt as a solution here.

beaugunderson commented 1 year ago

dnt not really a solution because it requires you to deno-fy the entire project... went down that path just to see what it would be like and it's a LOT. will think over just nuking CJS support unless someone can find the agreed-upon least work approach.

beaugunderson commented 11 months ago

9.0.0 should work everywhere... let me know if it doesn't for some reason but I tested with CJS/ESM/TS and all worked fine

laverdet commented 11 months ago

@beaugunderson I'm finding it doesn't work at all, under any circumstance. The distributed package only includes TypeScript source files, not the built js files:

-> % npm init -y
// [ ... ]

-> % npm install ip-address
added 3 packages, and audited 4 packages in 361ms

-> % node
Welcome to Node.js v20.1.0.
Type ".help" for more information.
> require('ip-address')
Uncaught:
Error: Cannot find module '/Users/marcel/tmp/node_modules/ip-address/dist/ip-address.js'. Please verify that the package.json has a valid "main" entry
    at tryPackage (node:internal/modules/cjs/loader:438:19)
    at Module._findPath (node:internal/modules/cjs/loader:687:18)
    at Module._resolveFilename (node:internal/modules/cjs/loader:1068:27)
    at Module._load (node:internal/modules/cjs/loader:928:27)
    at Module.require (node:internal/modules/cjs/loader:1149:19)
    at require (node:internal/modules/helpers:121:18) {
  code: 'MODULE_NOT_FOUND',
  path: '/Users/marcel/tmp/node_modules/ip-address/package.json',
  requestPath: 'ip-address'
}
> await import('ip-address')
Uncaught:
Error [ERR_MODULE_NOT_FOUND]: Cannot find package '/Users/marcel/tmp/node_modules/ip-address/' imported from /Users/marcel/tmp/repl
    at __node_internal_captureLargerStackTrace (node:internal/errors:490:5)
    at new NodeError (node:internal/errors:399:5)
    at legacyMainResolve (node:internal/modules/esm/resolve:194:9)
    at packageResolve (node:internal/modules/esm/resolve:770:14)
    at moduleResolve (node:internal/modules/esm/resolve:832:20)
    at defaultResolve (node:internal/modules/esm/resolve:1069:11)
    at DefaultModuleLoader.resolve (node:internal/modules/esm/loader:307:12)
    at DefaultModuleLoader.getModuleJob (node:internal/modules/esm/loader:156:32)
    at DefaultModuleLoader.import (node:internal/modules/esm/loader:266:12)
    at importModuleDynamically (node:repl:507:47)
    at importModuleDynamicallyWrapper (node:internal/vm/module:428:21)
    at importModuleDynamicallyCallback (node:internal/modules/esm/utils:87:14)
    at REPL2:1:33 {
  code: 'ERR_MODULE_NOT_FOUND'
}

-> % find node_modules/ip-address 
node_modules/ip-address
node_modules/ip-address/LICENSE
node_modules/ip-address/README.md
node_modules/ip-address/package.json
node_modules/ip-address/src
node_modules/ip-address/src/v6
node_modules/ip-address/src/v6/helpers.ts
node_modules/ip-address/src/v6/constants.ts
node_modules/ip-address/src/v6/regular-expressions.ts
node_modules/ip-address/src/address-error.ts
node_modules/ip-address/src/ipv6.ts
node_modules/ip-address/src/ip-address.ts
node_modules/ip-address/src/common.ts
node_modules/ip-address/src/v4
node_modules/ip-address/src/v4/constants.ts
node_modules/ip-address/src/ipv4.ts

I'm not sure how you published it but it doesn't look like the build step actually ran. I don't think this is the issue but your "prepublishOnly" script in package.json should be "prepack", which is more correct (will run on npm pack, instead of only npm publish). That wouldn't fix whatever happened with the version that you published, I just wanted to point it out.

When I run npm run build && npm run pack locally I get this:

-> % npm pack         
npm notice 
npm notice 📦  ip-address@9.0.0
npm notice === Tarball Contents === 
npm notice 1.1kB  LICENSE                             
npm notice 4.8kB  README.md                           
npm notice 179B   dist/address-error.d.ts             
npm notice 228B   dist/address-error.d.ts.map         
npm notice 423B   dist/address-error.js               
npm notice 337B   dist/address-error.js.map           
npm notice 370B   dist/common.d.ts                    
npm notice 393B   dist/common.d.ts.map                
npm notice 770B   dist/common.js                      
npm notice 672B   dist/common.js.map                  
npm notice 325B   dist/ip-address.d.ts                
npm notice 398B   dist/ip-address.d.ts.map            
npm notice 1.8kB  dist/ip-address.js                  
npm notice 296B   dist/ip-address.js.map              
npm notice 5.4kB  dist/ipv4.d.ts                      
npm notice 1.6kB  dist/ipv4.d.ts.map                  
npm notice 10.6kB dist/ipv4.js                        
npm notice 5.9kB  dist/ipv4.js.map                    
npm notice 12.2kB dist/ipv6.d.ts                      
npm notice 3.6kB  dist/ipv6.d.ts.map                  
npm notice 34.5kB dist/ipv6.js                        
npm notice 22.7kB dist/ipv6.js.map                    
npm notice 192B   dist/v4/constants.d.ts              
npm notice 243B   dist/v4/constants.d.ts.map          
npm notice 468B   dist/v4/constants.js                
npm notice 243B   dist/v4/constants.js.map            
npm notice 1.1kB  dist/v6/constants.d.ts              
npm notice 626B   dist/v6/constants.d.ts.map          
npm notice 2.6kB  dist/v6/constants.js                
npm notice 1.2kB  dist/v6/constants.js.map            
npm notice 629B   dist/v6/helpers.d.ts                
npm notice 417B   dist/v6/helpers.d.ts.map            
npm notice 1.7kB  dist/v6/helpers.js                  
npm notice 1.3kB  dist/v6/helpers.js.map              
npm notice 428B   dist/v6/regular-expressions.d.ts    
npm notice 445B   dist/v6/regular-expressions.d.ts.map
npm notice 4.0kB  dist/v6/regular-expressions.js      
npm notice 2.7kB  dist/v6/regular-expressions.js.map  
npm notice 2.2kB  package.json                        
npm notice 263B   src/address-error.ts                
npm notice 728B   src/common.ts                       
npm notice 260B   src/ip-address.ts                   
npm notice 8.9kB  src/ipv4.ts                         
npm notice 31.0kB src/ipv6.ts                         
npm notice 288B   src/v4/constants.ts                 
npm notice 2.4kB  src/v6/constants.ts                 
npm notice 1.5kB  src/v6/helpers.ts                   
npm notice 2.5kB  src/v6/regular-expressions.ts       
npm notice === Tarball Details === 
npm notice name:          ip-address                              
npm notice version:       9.0.0                                   
npm notice filename:      ip-address-9.0.0.tgz                    
npm notice package size:  36.4 kB                                 
npm notice unpacked size: 176.9 kB                                
npm notice shasum:        8815931d6ce471bab4f5dd41fe93966c21f6efd5
npm notice integrity:     sha512-4ocH/tDoFVANt[...]+1SzFk+DwtuVw==
npm notice total files:   48                                      
npm notice 
ip-address-9.0.0.tgz

This doesn't match the published npm version.

beaugunderson commented 11 months ago

Yes, sorry about that! Fixed in 9.0.5. incremental: true was the culprit; the prepublish step wasn't regenerating the dist files.

On Mon, Sep 25, 2023 at 8:00 AM, Marcel Laverdet @.***> wrote:

@beaugunderson https://github.com/beaugunderson I'm finding it doesn't work all, under any circumstance. The distributed package only includes TypeScript source files, not the built js files:

-> % npm init -y // [ ... ]

-> % npm install ip-address added 3 packages, and audited 4 packages in 361ms

-> % node Welcome to Node.js v20.1.0. Type ".help" for more information.

require('ip-address') Uncaught: Error: Cannot find module '/Users/marcel/tmp/node_modules/ip-address/dist/ip-address.js'. Please verify that the package.json has a valid "main" entry at tryPackage (node:internal/modules/cjs/loader:438:19) at Module._findPath (node:internal/modules/cjs/loader:687:18) at Module._resolveFilename (node:internal/modules/cjs/loader:1068:27) at Module._load (node:internal/modules/cjs/loader:928:27) at Module.require (node:internal/modules/cjs/loader:1149:19) at require (node:internal/modules/helpers:121:18) { code: 'MODULE_NOT_FOUND', path: '/Users/marcel/tmp/node_modules/ip-address/package.json', requestPath: 'ip-address' } await import('ip-address') Uncaught: Error [ERR_MODULE_NOT_FOUND]: Cannot find package '/Users/marcel/tmp/node_modules/ip-address/' imported from /Users/marcel/tmp/repl at __node_internal_captureLargerStackTrace (node:internal/errors:490:5) at new NodeError (node:internal/errors:399:5) at legacyMainResolve (node:internal/modules/esm/resolve:194:9) at packageResolve (node:internal/modules/esm/resolve:770:14) at moduleResolve (node:internal/modules/esm/resolve:832:20) at defaultResolve (node:internal/modules/esm/resolve:1069:11) at DefaultModuleLoader.resolve (node:internal/modules/esm/loader:307:12) at DefaultModuleLoader.getModuleJob (node:internal/modules/esm/loader:156:32) at DefaultModuleLoader.import (node:internal/modules/esm/loader:266:12) at importModuleDynamically (node:repl:507:47) at importModuleDynamicallyWrapper (node:internal/vm/module:428:21) at importModuleDynamicallyCallback (node:internal/modules/esm/utils:87:14) at REPL2:1:33 { code: 'ERR_MODULE_NOT_FOUND' }

-> % find node_modules/ip-address node_modules/ip-address node_modules/ip-address/LICENSEnode_modules/ip-address/README.md node_modules/ip-address/package.json node_modules/ip-address/src node_modules/ip-address/src/v6 node_modules/ip-address/src/v6/helpers.ts node_modules/ip-address/src/v6/constants.ts node_modules/ip-address/src/v6/regular-expressions.ts node_modules/ip-address/src/address-error.ts node_modules/ip-address/src/ipv6.ts node_modules/ip-address/src/ip-address.ts node_modules/ip-address/src/common.ts node_modules/ip-address/src/v4 node_modules/ip-address/src/v4/constants.ts node_modules/ip-address/src/ipv4.ts

I'm not sure how you published it but it doesn't look the build step actually ran. I don't think this is the issue but your "prepublishOnly" script in package.json should be "prepack", which is more correct (will run on npm pack, instead of only npm publish). That wouldn't fix whatever happened with the version that you published, I just wanted to point it out.

When I run npm run build && npm run pack locally I get this:

-> % npm pack npm notice npm notice [image: 📦] @.*** npm notice === Tarball Contents === npm notice 1.1kB LICENSE npm notice 4.8kB README.md http://readme.md/ npm notice 179B dist/address-error.d.ts npm notice 228B dist/address-error.d.ts.map npm notice 423B dist/address-error.js npm notice 337B dist/address-error.js.map npm notice 370B dist/common.d.ts npm notice 393B dist/common.d.ts.map npm notice 770B dist/common.js npm notice 672B dist/common.js.map npm notice 325B dist/ip-address.d.ts npm notice 398B dist/ip-address.d.ts.map npm notice 1.8kB dist/ip-address.js npm notice 296B dist/ip-address.js.map npm notice 5.4kB dist/ipv4.d.ts npm notice 1.6kB dist/ipv4.d.ts.map npm notice 10.6kB dist/ipv4.js npm notice 5.9kB dist/ipv4.js.map npm notice 12.2kB dist/ipv6.d.ts npm notice 3.6kB dist/ipv6.d.ts.map npm notice 34.5kB dist/ipv6.js npm notice 22.7kB dist/ipv6.js.map npm notice 192B dist/v4/constants.d.ts npm notice 243B dist/v4/constants.d.ts.map npm notice 468B dist/v4/constants.js npm notice 243B dist/v4/constants.js.map npm notice 1.1kB dist/v6/constants.d.ts npm notice 626B dist/v6/constants.d.ts.map npm notice 2.6kB dist/v6/constants.js npm notice 1.2kB dist/v6/constants.js.map npm notice 629B dist/v6/helpers.d.ts npm notice 417B dist/v6/helpers.d.ts.map npm notice 1.7kB dist/v6/helpers.js npm notice 1.3kB dist/v6/helpers.js.map npm notice 428B dist/v6/regular-expressions.d.ts npm notice 445B dist/v6/regular-expressions.d.ts.map npm notice 4.0kB dist/v6/regular-expressions.js npm notice 2.7kB dist/v6/regular-expressions.js.map npm notice 2.2kB package.json npm notice 263B src/address-error.ts npm notice 728B src/common.ts npm notice 260B src/ip-address.ts npm notice 8.9kB src/ipv4.ts npm notice 31.0kB src/ipv6.ts npm notice 288B src/v4/constants.ts npm notice 2.4kB src/v6/constants.ts npm notice 1.5kB src/v6/helpers.ts npm notice 2.5kB src/v6/regular-expressions.ts npm notice === Tarball Details === npm notice name: ip-address npm notice version: 9.0.0 npm notice filename: ip-address-9.0.0.tgz npm notice package size: 36.4 kB npm notice unpacked size: 176.9 kB npm notice shasum: 8815931d6ce471bab4f5dd41fe93966c21f6efd5 npm notice integrity: sha512-4ocH/tDoFVANt[...]+1SzFk+DwtuVw== npm notice total files: 48 npm notice ip-address-9.0.0.tgz

This doesn't match the published npm version.

— Reply to this email directly, view it on GitHub https://github.com/beaugunderson/ip-address/issues/153#issuecomment-1733898307, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAPCX6E7SPHLDEVNV6Q4BDX4GMB3ANCNFSM5WQZNCPA . You are receiving this because you were mentioned.Message ID: @.***>

beaugunderson / ip-address

Please add {"type": "module"} to package.json #153

.mjs / ECMAScript workaround example: