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

Minify globals.json #198

Open
dosisod opened this issue Apr 18, 2023 · 6 comments
Open

Minify globals.json #198

dosisod opened this issue Apr 18, 2023 · 6 comments

Comments

@dosisod
Copy link

dosisod commented Apr 18, 2023

We could make this package a lot smaller by minifying the globals.json file. Considering that this package is downloaded 53 million (!) times a week (according to NPM), making it as small as possible will save bandwidth and storage.

To start, here is the size of the package based off the main branch (only including the relevant lines):

$ npm pack
npm notice 42.2kB globals.json
npm notice package size:  9.4 kB
npm notice unpacked size: 46.6 kB

Simply by stripping white space, newlines, etc., from the JSON file we can decrease the file size by ~7.1KB:

$ npm pack
npm notice 35.1kB globals.json
npm notice package size:  9.3 kB
npm notice unpacked size: 39.5 kB

This is good, but we can do better! If we replace true and false with 1 and 0 respectively we can reduce the file size even further:

$ npm pack
npm notice 28.3kB globals.json
npm notice package size:  9.0 kB
npm notice unpacked size: 32.7 kB

This last method will require that we change the index.js file to convert 1 and 0 with true and false to maintain backwards compatibility, which might be more trouble than it is worth.

I don't know what the deployment process for this project looks like (I don't see any GitHub Action workflows or anything), but I might suggest that, at the very least, the globals.json file be minified before publishing. I would be willing to open a PR for the second method if we decide it would be worth it.

This issue also could be applicable to the builtin-modules package as well.

Thanks!

@silverwind
Copy link
Contributor

silverwind commented Sep 20, 2023

The index.js wrapper can be removed as well. One can just set exports and main to the json file. As far as I'm aware, this works universally in all node versions and in both CJS and ESM imports.

@silverwind
Copy link
Contributor

I guess the ideal course of action would be a index.yaml for editing which is then built into a minified index.json for packaging. I'm just not sure whether typescript definitions would work with packages that export a json file.

@fregante
Copy link

works universally in all node versions and in both CJS and ESM imports

globals/package.json

Lines 13 to 15 in d887e4c

"engines": {
"node": ">=8"
},

In a rare exception, this package supports node 8 and up, so it's incompatible with ESM imports between Node 12 and Node 16 (where ESM is supported, but assert {type: "json"} isn't)

@silverwind
Copy link
Contributor

You don't need a import assertion to import a module with a .json file in main/exports. The node module loader apparently does some magic so it works without.

@fregante
Copy link

fregante commented Sep 26, 2023

You don't need a import assertion to import a module with a .json file in main/exports

That's not true. Also this request is off-topic so I'll stop here.

https://unpkg.com/browse/test-test-test-json-import-will-delete-this-package@0.0.0/package.json

{
  "name": "test-test-test-json-import-will-delete-this-package",
  "version": "0.0.0",
  "exports": "./package.json"
}
// index.mjs
import x from 'test-test-test-json-import-will-delete-this-package';

console.log(x);
node index.mjs 
node:internal/errors:490
    ErrorCaptureStackTrace(err);
    ^

TypeError [ERR_IMPORT_ASSERTION_TYPE_MISSING]: Module "./node_modules/test-test-test-json-import-will-delete-this-package/package.json" needs an import assertion of type "json"
    at new NodeError (node:internal/errors:399:5)
    at validateAssertions (node:internal/modules/esm/assert:94:15)
    at defaultLoad (node:internal/modules/esm/load:86:3)
    at DefaultModuleLoader.load (node:internal/modules/esm/loader:320:26)
    at DefaultModuleLoader.moduleProvider (node:internal/modules/esm/loader:195:22)
    at new ModuleJob (node:internal/modules/esm/module_job:63:26)
    at #createModuleJob (node:internal/modules/esm/loader:219:17)
    at DefaultModuleLoader.getJobFromResolveResult (node:internal/modules/esm/loader:172:34)
    at DefaultModuleLoader.getModuleJob (node:internal/modules/esm/loader:157:17)
    at ModuleWrap.<anonymous> (node:internal/modules/esm/module_job:76:33) {
  code: 'ERR_IMPORT_ASSERTION_TYPE_MISSING'
}

Node.js v20.1.0

@silverwind
Copy link
Contributor

Interesting, likely I wasn't aware of this as I import all my json modules in a bundler and/or typscript which supports json without assertion. So yes, converting this module to json would be a breaking change for node users.

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

No branches or pull requests

3 participants