Closed userquin closed 3 years ago
Maybe we can generate single esm/cjs
modules for each collection similar to @mdi/js
Do you mean an entire JSON data as one export? Or many named exports for each icon that include only data?
For each json file one module with multiple exports, one per icon with the meta info, I don't now if can be done or much more code than flat json file.
I mean this:
// Material Design Icons v6.1.95
export var mdiAbTesting = "M4 2A2 2 0 0 0 2 4V12H4V8H6V12H8V4A2 2 0 0 0 6 2H4M4 4H6V6H4M22 15.5V14A2 2 0 0 0 20 12H16V22H20A2 2 0 0 0 22 20V18.5A1.54 1.54 0 0 0 20.5 17A1.54 1.54 0 0 0 22 15.5M20 20H18V18H20V20M20 16H18V14H20M5.79 21.61L4.21 20.39L18.21 2.39L19.79 3.61Z";
export var mdiAbacus = "M5 5H7V11H5V5M10 5H8V11H10V5M5 19H7V13H5V19M10 13H8V19H10V17H15V15H10V13M2 21H4V3H2V21M20 3V7H13V5H11V11H13V9H20V15H18V13H16V19H18V17H20V21H22V3H20Z";
export var mdiAbjadArabic = "M12 4C10.08 4 8.5 5.58 8.5 7.5C8.5 8.43 8.88 9.28 9.5 9.91C7.97 10.91 7 12.62 7 14.5C7 17.53 9.47 20 12.5 20C14.26 20 16 19.54 17.5 18.66L16.5 16.93C15.28 17.63 13.9 18 12.5 18C10.56 18 9 16.45 9 14.5C9 12.91 10.06 11.53 11.59 11.12L16.8 9.72L16.28 7.79L11.83 9C11.08 8.9 10.5 8.28 10.5 7.5C10.5 6.66 11.16 6 12 6C12.26 6 12.5 6.07 12.75 6.2L13.75 4.47C13.22 4.16 12.61 4 12 4Z";
but with meta info if required, maybe we can export the meta as another export.
For each json file on json
folder.
They do exist. For example, `@iconify-icons/mdi'. Because there are so many icons, each icon is in its own file:
import mdiAbTesting from '@iconify-icons/mdi/ab-testing';
Forgot previous comment, it is done on this another repo.
I've included some compatibility on IconifyInfo
see author
, license
and palette
.
Also forgot to mention that collection
(new) and collections
are now async
and thorws
error, previous version collections
returns null
on error.
can you create a next
branch on json-tools
?
EDIT: seems to be also created
I will add some test with jest
(once finished initial PR for @iconify/json-tools
)
I also add jest
on @iconify/json-tools
instead mocha
.
Sorry for delay, needed to finish large chunk of urgent project first.
Thank you very much for this pull request. ESM support is a must have, but I didn't realise how much tooling has changed this year to support it. So now I'm going to move everything to ES and new tools.
Few things I don't understand:
Issues:
collections()
returns object, where key is prefix, value is info, not array: Promise<Record<string, IconifyMetaData>>
@cyberalien I need to cleanup such things... just reusing it from another project ;) It is the initial proposal, I'm also very busy at work.
In fact, later, I'll move lib
to src
and change finder.ts
to index.ts
...
@cyberalien about the methods signature, I'm moving including some logic from unplugin-icons
here (1 or 2 small methods), so I need to review a few things.
@cyberalien This PR should be a draft, if you can convert it to draft...
Thanks! Changed to draft
@cyberalien can you take a look at changes and the refactor? I've removed some internals, and create a type for collections
result now lookupCollections
or just keep the original names?
I'll add jest
and some minimal tests.
@cyberalien this weekend I'll try to also add jest
forcjs
also need to review how to use directly the ts
from the source, right now the test is using the index
module from the dist
folder.
On older versions of jest
I can run the tests importing directly the ts
files on the ts tests scripts
instead having to build the dist and I don't now why, maybe some jest plugin
required.
@cyberalien can you take a look at changes and the refactor? I've removed some internals, and create a type for
collections
result nowlookupCollections
or just keep the original names?
Looks good!
@cyberalien I just remove the useESM: true
on a copy of the original, now we have 2 test scripts: test-esm
and test-cjs
.
I have test the cjs
running test-cjs
, removing the mjs
from dist
folder, remove the yarn build
from the script and run it again.
I'll add a few tests and so we can remove the draft status.
NOTE ABOUT TEST: in both cases (test-cjs
and test-esm
) tests are using cjs
module, the tests are compiled to cjs
and mjs
respectivelly but importing the dist/index.js
module (cjs
).
I can test the mjs
if I create a ts
file and then run with cross-env node --experimental-specifier-resolution=node --loader ts-node/esm src/xxx.ts
.
I have not changed the package.json
to esm
("type": "module"
) to test it: maybe it is a good idea to change it.
I'll ask about the type module to @antfu
tmr.
EDIT: jest
fails to load if I add "type": "module"
to package.json
.
I'm changing some configuration to test cjs and esm, so don't merge yet: the test here are not important but used on @iconify/json-tools
There is a downside with last commit: the tests are duplicated.
The test-cjs
script will compile the tests to cjs
and will use the cjs
from the dist
, while the test-esm
will import index.ts
compiling the test and the index.ts
to mjs
.
We need to figure out how to bypass this, the problem is that I'm using node 15
and there is no way to use the cjs
on jest
if we import from index.ts
to do the tests (maybe configuring babel-jest
on cjs jest
configuration):
yarn run v1.22.10
$ jest --clearCache && jest --config=jest.cjs.config.ts
Cleared C:\Users\userquin\AppData\Local\Temp\jest
FAIL src/locate.test.ts
● Test suite failed to run
src/index.ts:395:87 - error TS1343: The 'import.meta' meta-property is only allowed when the '--module' option is 'es2020', 'esnext', or 'system'.
395 const _dirname = typeof __dirname !== 'undefined' ? __dirname : dirname(fileURLToPath(import.meta.url))
~~~~~~~~~~~
FAIL src/loadCollection.test.ts
● Test suite failed to run
src/index.ts:395:87 - error TS1343: The 'import.meta' meta-property is only allowed when the '--module' option is 'es2020', 'esnext', or 'system'.
395 const _dirname = typeof __dirname !== 'undefined' ? __dirname : dirname(fileURLToPath(import.meta.url))
~~~~~~~~~~~
Test Suites: 2 failed, 2 total
Tests: 0 total
Snapshots: 0 total
Time: 2.581 s
Ran all test suites.
I will do a few tests more to avoid duplicating the tests...
ready, only one test codebase, but we need to include esm and cjs tests importing from .
and ../dist
respectively, see the esm
and cjs
modules, calling to the test passing the required imports to the test.
I'm a bit confused about unit test. According to documentation, jest is supposed to test ES module. But in reality it runs CommonJS file.
You can test it by adding console.log(__filename);
to src/index.ts
before running test. Test will output dist/index.js
edit: actually, that test code should throw an error because __filename
is not available when running ES modules in Node.js. But it doesn't and no matter what I try, Jest always runs CommonJS code. So far only Jasmine seems to be able to test ES modules properly.
You've included @antfu/eslint-config
, which has linting rules for things like use of spaces, semicolons and other editor stuff, but didn't include config for editor to use that formatting. Can you include matching .editorconfig
and/or Prettier config?
upath
cannot be used here as dependency. It works only with CommonJS files, not ES modules. Current test doesn't show it because tests are actually ran as CommonJS tests.
To test it correctly, create file _test.mjs
:
import { lookupCollections } from './dist/index.mjs';
lookupCollections().then(console.log).catch(console.error);
and run it: env NODE_OPTIONS=--experimental-vm-modules node _test.mjs
It will load generated index.mjs
and will throw an error.
Replacing upath
with path
fixes it.
I'm a bit confused about unit test. According to documentation, jest is supposed to test ES module. But in reality it runs CommonJS file.
You can test it by adding
console.log(__filename);
tosrc/index.ts
before running test. Test will outputdist/index.js
edit: actually, that test code should throw an error because
__filename
is not available when running ES modules in Node.js. But it doesn't and no matter what I try, Jest always runs CommonJS code. So far only Jasmine seems to be able to test ES modules properly.
The new version is a bit confusing.
I look at what it generates to see if it is using cjs
ormjs
. If you run one of the tests, look at the temporary directory and locate the entries in the different directories it generates and then run the other test.
About the __filename
we are including isomorphic __filename
based on isomorphic dirname and so both cjs
and mjs
output will work as expected.
When used on target project, it will depend on "type": "module"
package.json
option entry (if missing will use cjs
otherwise mjs
.
You've included
@antfu/eslint-config
, which has linting rules for things like use of spaces, semicolons and other editor stuff, but didn't include config for editor to use that formatting. Can you include matching.editorconfig
and/or Prettier config?
Yeah, I'm using IntelliJ (WebStorm)
ide, I need to push some missing files...
upath
cannot be used here as dependency. It works only with CommonJS files, not ES modules. Current test doesn't show it because tests are actually ran as CommonJS tests.To test it correctly, create file
_test.mjs
:import { lookupCollections } from './dist/index.mjs'; lookupCollections().then(console.log).catch(console.error);
and run it:
env NODE_OPTIONS=--experimental-vm-modules node _test.mjs
It will load generated
index.mjs
and will throw an error.Replacing
upath
withpath
fixes it.
Maybe the node version should be 16 to test ESM
: I'm using 15.11.0
but I can swtich to 16.7.0
to just test again.
This is because our package.json
is not "type": "module"
and so will use cjs
. Maybe we need to add some example directories for cjs
and esm
tests with their own package.json
file.
You can see for example this repo esbuild-node-loader, another experimental loader. The 2 examples are using "type": "module"
on their package.json
files.
/cc @antfu can you give us some input hint about this
Use https://github.com/unjs/pathe, I believe @pi0 was trying to solve the same problem 😄
I've tested changing type to module in Node 16, so this is what happens:
upath
is still not usable. It wasn't built for it.tsup
change: .js
becomes ES file, .cjs
is a new CommonJS file, .mjs
no longer exists.pathe
seems to be a good solution.
I've tested changing type to module in Node 16, so this is what happens:
* Jest does test ES modules properly. * `upath` is still not usable. It wasn't built for it. * Exports generated by `tsup` change: `.js` becomes ES file, `.cjs` is a new CommonJS file, `.mjs` no longer exists.
pathe
seems to be a good solution.
I'm changing it right now...
There is few more known issues with pathe but indeed it can resolve esm compatibility issue of upath :)
Regarding the isomorphic way of accessing __dirname
, you can also use unjs/mlly
There is few more known issues with pathe but indeed it can resolve esm compatibility issue of upath :)
Regarding the isomorphic way of accessing
__dirname
, you can also use unjs/mlly
There is something weird, we cannot use resolve
from pathe
on windows it is trying to resolve the to
path segments as file:/<UNIT>...
:
For example, trying to resolve const dir = resolve(_dirname, '..')
will output F:\work\projects\quini\GitHub\iconify-projects\collections-json/F:/work/projects/quini/GitHub/iconify-projects/collections-json
, I just replace resolve
with join
and seems to work properly.
We have a similar problem on esbuild-node-loader
, the windows protocol cannot be removed when resolving: https://github.com/antfu/esbuild-node-loader/blob/main/loader.mjs#L48
EDIT: the absolute paths is not valid on windows: file:///<UNIT>/...
should be used since /<UNIT>:/...
is not valid on windows.
I've pushed some changes, I have left some tests on src/index.ts
on line 399
@cyberalien can you test with new changes?
There is few more known issues with pathe but indeed it can resolve esm compatibility issue of upath :)
Regarding the isomorphic way of accessing
__dirname
, you can also use unjs/mlly
I'll take a look later...
I've tested changing type to module in Node 16, so this is what happens:
* Jest does test ES modules properly. * `upath` is still not usable. It wasn't built for it. * Exports generated by `tsup` change: `.js` becomes ES file, `.cjs` is a new CommonJS file, `.mjs` no longer exists.
pathe
seems to be a good solution.
Yeah, if we add "type": "module"
to package.json
then tsup
change the extension outputs, see tsup bundle formats
@cyberalien I'm more confortable using pnpm workspaces
instead lerna
, so I will switch to pnpm
to add 2 project examples.
I'll switch on project, can I proceed with that change?
lerna
is outdated and not really maintained, so moving on from it would be a good idea.
However switching this repo might not be possible because it is also used by PHP packages. Last time I checked, Packagist (PHP version of NPM) did not work with monorepos, though things could have changed since then so it is worth trying. In worst case repo will have to be reverted to current structure.
I've pushed some changes, I have left some tests on
src/index.ts
on line 399@cyberalien can you test with new changes?
Tested with Node 16. All tests are running in correct mode: -esm
tests do test real ESM modules, -cjs
tests do test CJS modules.
lerna
is outdated and not really maintained, so moving on from it would be a good idea.However switching this repo might not be possible because it is also used by PHP packages. Last time I checked, Packagist (PHP version of NPM) did not work with monorepos, though things could have changed since then so it is worth trying. In worst case repo will have to be reverted to current structure.
VSCode
working correctly (I added some config to it)?.
About PHP, we can keep old version, so ppl using php can still use it. I'm preparing a new branch on my repo to also generate icon-set
variant with pnpm workspaces
. Can you share the changes on issue #15?.
lerna
is outdated and not really maintained, so moving on from it would be a good idea. However switching this repo might not be possible because it is also used by PHP packages. Last time I checked, Packagist (PHP version of NPM) did not work with monorepos, though things could have changed since then so it is worth trying. In worst case repo will have to be reverted to current structure.
VSCode
working correctly (I added some config to it)?.About PHP, we can keep old version, so ppl using php can still use it. I'm preparing a new branch on my repo to also generate
icon-set
variant withpnpm workspaces
. Can you share the changes on issue #15?.
Sure. There are 2 files: info.ts and split.ts.
This is source for info.ts: https://github.com/iconify/icon-finder/blob/core/src/converters/info.ts
and this is split.ts:
import fs from 'fs';
import type {
IconifyChars,
IconifyJSON,
IconifyMetaData,
} from '@iconify/types';
import { dataToCollectionInfo } from './info';
const splitPackageName = '@iconify-json/{prefix}';
function saveJSON(file: string, data: unknown) {
fs.writeFileSync(file, JSON.stringify(data, null, '\t') + '\n');
}
function importedName(attr: string): string {
return 'imported' + attr.slice(0, 1).toUpperCase() + attr.slice(1);
}
function addJSDoc(list: string[], values: string | string[]) {
list.push('/**');
(values instanceof Array ? values : [values]).forEach((item) => {
list.push(' * ' + item);
});
list.push(' */');
}
const metadataKeys: (keyof IconifyMetaData)[] = [
'categories',
'themes',
'prefixes',
'suffixes',
];
const types: Record<ExportKeys, string> = {
info: 'IconifyInfo',
icons: 'IconifyJSON',
metadata: 'IconifyMetaData',
chars: 'IconifyChars',
};
type ExportKeys = 'info' | 'icons' | 'metadata' | 'chars';
/**
* Split JSON
*/
export function splitJSON(data: unknown, outputDir: string, version: string) {
const fullJSON = data as IconifyJSON;
const prefix = fullJSON.prefix;
const contents: Record<ExportKeys, boolean> = {
info: true,
icons: true,
metadata: false,
chars: false,
};
// Get info
const info = dataToCollectionInfo(fullJSON.info, prefix);
if (!info) {
throw new Error('Cannot convert info!');
}
saveJSON(outputDir + '/info.json', info);
// Get icon set
const iconSet: IconifyJSON = {
prefix,
icons: fullJSON.icons,
};
if (fullJSON.aliases) {
iconSet.aliases = fullJSON.aliases;
}
saveJSON(outputDir + '/icons.json', iconSet);
// Get metadata
const metaData: IconifyMetaData = {};
metadataKeys.forEach((attr) => {
if (fullJSON[attr]) {
(metaData as Record<string, unknown>)[attr] = fullJSON[attr];
}
});
saveJSON(outputDir + '/metadata.json', metaData);
if (Object.keys(metaData).length) {
contents['metadata'] = true;
}
// Characters
const chars: IconifyChars = fullJSON.chars ? fullJSON.chars : {};
saveJSON(outputDir + '/chars.json', chars);
if (Object.keys(chars).length) {
contents['chars'] = true;
}
// Generate exports
const cjsImports: string[] = [];
const cjsExports: string[] = [];
const mjsImports: string[] = [];
const mjsConsts: string[] = [];
const mjsExports: string[] = [];
const dtsTypes: Set<string> = new Set();
const dtsDeclares: string[] = [];
Object.keys(types).forEach((attr) => {
const hasContent = contents[attr as ExportKeys];
const type = types[attr as ExportKeys];
dtsTypes.add(type);
dtsDeclares.push(`export declare const ${attr}: ${type};`);
cjsExports.push(`exports.${attr} = ${attr};`);
mjsExports.push(attr);
if (!hasContent) {
// Missing item
addJSDoc(
cjsImports,
`@type {import('@iconify/types').${type}} ${attr}`
);
cjsImports.push(`const ${attr} = {};\n`);
addJSDoc(
mjsConsts,
`@type {import('@iconify/types').${type}} ${attr}`
);
mjsConsts.push(`const ${attr} = {};\n`);
return;
}
const target = `./${attr}.json`;
addJSDoc(
cjsImports,
`@type {import('@iconify/types').${type}} ${attr}`
);
cjsImports.push(`const ${attr} = require('${target}');\n`);
mjsImports.push(`import ${importedName(attr)} from '${target}';`);
addJSDoc(mjsConsts, `@type {import('@iconify/types').${type}} ${attr}`);
mjsConsts.push(`const ${attr} = ${importedName(attr)};\n`);
});
// Generate CJS index file
const cjsContent = cjsImports.concat(cjsExports);
fs.writeFileSync(
outputDir + '/index.js',
cjsContent.join('\n') + '\n',
'utf8'
);
// Generate MJS index file
const mjsContent = mjsImports.concat([''], mjsConsts, [
`export { ${mjsExports.join(', ')} };`,
]);
fs.writeFileSync(
outputDir + '/index.mjs',
mjsContent.join('\n') + '\n',
'utf8'
);
// Generate types file
const usedTypes = Array.from(dtsTypes);
const typesData = [
`import type { ${usedTypes.join(', ')} } from '@iconify/types';`,
'',
`export { ${usedTypes.join(', ')} };`,
'',
].concat(dtsDeclares);
fs.writeFileSync(
outputDir + '/index.d.ts',
typesData.join('\n') + '\n',
'utf8'
);
// Generate package.json
const packageExports: Record<string, string | Record<string, string>> = {
'./*': './*',
'.': {
require: './index.js',
import: './index.mjs',
},
};
const packageJSONIconSet: Partial<Record<ExportKeys, string>> = {};
Object.keys(contents)
.filter((key) => contents[key as ExportKeys])
.forEach((attr) => {
packageJSONIconSet[attr as ExportKeys] = attr + '.json';
packageExports[`./${attr}.json`] = `./${attr}.json`;
});
const packageJSON = {
name: splitPackageName.replace('{prefix}', prefix),
description: `${info.name} icon set in Iconify JSON format`,
version,
iconSetVersion: info.version,
main: 'index.js',
module: 'index.mjs',
types: 'index.d.ts',
exports: packageExports,
iconSet: packageJSONIconSet,
dependencies: {
'@iconify/types': '^1.0.7',
},
};
saveJSON(outputDir + '/package.json', packageJSON);
}
Function is synchronous, I didn't bother making it async because script isn't meant to be parsing multiple icon sets at the same time - it might require too much memory.
Example usage (simple use case, a bit messy, but enough to understand how to use it):
const fs = require('fs');
const { locate } = require('@iconify/json');
const { splitJSON } = require('./lib/split');
const filename = locate('mdi');
const data = JSON.parse(fs.readFileSync(filename, 'utf8'));
const outputDir = __dirname + '/test';
splitJSON(data, outputDir, '1.0.0');
So about ESM support.
Thank you very much for opening this issue and help. This week I've been rebuilding everything to generate and use proper ES modules and it has been more useful than I imagined.
@iconify/utils
now uses ES, so if you are using @iconify/json-tools
in your projects, I suggest switching to @iconify/utils
. It offers the same functionality, but as functions instead of classes and written with correct types and now supports ES modules.More work to be done, but so far it has been massive improvement. Thank you!
@cyberalien thanks for the input, I am happy to know that my PR were helpful to you to improve your set of libraries, the next step will be to integrate it into unplugin-icons
.
@cyberalien can you ping me here when available? Then I'll close this PR.
Yep, it looks good and I'll merge it.
@cyberalien there is a problem with types since we have @iconify/internal-types
and tsup
will not include the types, just keeps use export * from '@iconify/internal-types'
, but since this is not published the we have the problem.
I have included types declared on
iconify
types.