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.

Sign up for GitHub

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

Jump to bottom

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

Closed
laverdet opened this issue May 20, 2022 · 22 comments
Closed

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

laverdet opened this issue May 20, 2022 · 22 comments

Comments

@laverdet
Copy link

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
Copy link

maxpain commented Jul 4, 2022

The same problem

@maxpain
Copy link

maxpain commented Jul 4, 2022

@bardiharborow could you please look into this?

@beaugunderson
Copy link
Owner

this was supposed to do this for our hybrid approach:

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

@laverdet
Copy link
Author

laverdet commented Jul 4, 2022

@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.

@laverdet laverdet mentioned this issue Jul 4, 2022
Closed
@beaugunderson
Copy link
Owner

beaugunderson commented Jul 5, 2022 via email

@laverdet
Copy link
Author

laverdet commented Jul 6, 2022

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
Copy link
Author

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
Copy link

maxpain commented Aug 7, 2022

Any updates on this?

@maxpain
Copy link

maxpain commented Aug 15, 2022

Up

@bennettp123 bennettp123 mentioned this issue Jan 15, 2023
Closed
@bennettp123
Copy link

☝🏻@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! 😄

This was referenced Jan 16, 2023
Merged
Merged
@Towerism
Copy link

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

@mkraenz mkraenz mentioned this issue Feb 6, 2023
Closed
@thib3113
Copy link

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 : #143 . So I can use your fork without creating a new one :) .

@Abdull
Copy link

Abdull commented Sep 13, 2023

(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
Copy link
Author

laverdet commented Sep 13, 2023 •
edited
Loading

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
Copy link
Owner

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
Copy link

@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
Copy link
Author

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: node-fetch/node-fetch@b50fbc1

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
Copy link
Owner

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:

image

I'll look at dnt as a solution here.

@beaugunderson
Copy link
Owner

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
Copy link
Owner

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
Copy link
Author

laverdet commented Sep 25, 2023 •
edited
Loading

@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 📦  [email protected]
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
Copy link
Owner

beaugunderson commented Sep 26, 2023 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add support for ES Modules bennettp123/ip-address
7 participants
@beaugunderson @laverdet @Abdull @bennettp123 @Towerism @thib3113 @maxpain