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

feat: export version constant #2016

Merged
merged 1 commit into from
May 27, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .github/workflows/main.yml
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ jobs:
- run: yarn install
- run: yarn playwright install --with-deps chromium
- run: yarn test
- run: yarn test-bundles
- run: yarn test:bundles
regression:
name: Test regressions
runs-on: ubuntu-latest
Expand All @@ -65,4 +65,4 @@ jobs:
cache: yarn
- run: yarn install
- run: yarn playwright install --with-deps chromium
- run: yarn test-regression
- run: yarn test:regression
2 changes: 1 addition & 1 deletion eslint.config.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ export default [
},
{
languageOptions: {
ecmaVersion: 2021,
ecmaVersion: 'latest',
globals: {
...globals.nodeBuiltin,
},
Expand Down
4 changes: 3 additions & 1 deletion lib/svgo-node.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import os from 'os';
import fs from 'fs';
import { pathToFileURL } from 'url';
import path from 'path';
import { optimize as optimizeAgnostic } from './svgo.js';
import { VERSION, optimize as optimizeAgnostic } from './svgo.js';

const importConfig = async (configFile) => {
// dynamic import expects file url instead of path and may fail
Expand All @@ -25,6 +25,8 @@ const isFile = async (file) => {
}
};

export { VERSION };

export const loadConfig = async (configFile, cwd = process.cwd()) => {
if (configFile != null) {
if (path.isAbsolute(configFile)) {
Expand Down
3 changes: 3 additions & 0 deletions lib/svgo.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -52,5 +52,8 @@ type Output = {
data: string;
};

/** Installed version of SVGO. */
export declare const VERSION: string;

/** The core of SVGO */
export declare function optimize(input: string, config?: Config): Output;
3 changes: 3 additions & 0 deletions lib/svgo.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { stringifySvg } from './stringifier.js';
import { builtin } from './builtin.js';
import { invokePlugins } from './svgo/plugins.js';
import { encodeSVGDatauri } from './svgo/tools.js';
import { VERSION } from './version.js';

const pluginsMap = {};
for (const plugin of builtin) {
Expand Down Expand Up @@ -45,6 +46,8 @@ const resolvePluginConfig = (plugin) => {
return null;
};

export { VERSION };

export const optimize = (input, config) => {
if (config == null) {
config = {};
Expand Down
2 changes: 2 additions & 0 deletions lib/version.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
/** Version of SVGO. */
export const VERSION = '4.0.0';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wondering why don't you just require package.json? It seems simpler...

Also, mind you that requires come with a penalty when in CJS; modules is a different case since they are inherently a lot faster when loading.

Copy link
Member Author

@SethFalco SethFalco May 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. ESM doesn't have stable support for importing JSON files yet. (When it does, we'll migrate to this!)
  2. When using require, Rollup isn't bundling/inlining JSON files, which breaks the browser build. (Tries to import package.json rather than inlining the JSON at build time, while there may be a fix for that I'm also concerned that it will bundle the whole package.json instead of just the version which I also don't want.)
  3. Don't want to take an approach with reading it from fs as this would break the browser version where fs isn't available.

Most other projects that export a version appear to take a similar approach, which is what I referenced when I did this solution:

I've only seen 2 projects so far take your approach with require, but neither is applicable in our case:

Feature parity between Node.js and browser is important for SVGO, and this seemed like the best approach for that. Then I opted for a Node.js script instead of a gulpfile or grunt task, just because we don't have too much going on yet.

If you have a better idea, you're welcome to pitch it though! I was waiting before merging, specifically to see if others would propose a better approach.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess I didn't consider the browser bundle, although I'm pretty sure there are ways to do it.

I guess your solution does the job too, I was just thinking out loud in case there was a simpler way :)

16 changes: 8 additions & 8 deletions package.json
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
{
"packageManager": "yarn@3.8.2",
"name": "svgo",
"version": "3.3.1",
"version": "4.0.0",
"description": "SVGO is a Node.js library and command-line application for optimizing vector images.",
"license": "MIT",
"type": "module",
Expand Down Expand Up @@ -85,15 +85,15 @@
"node": ">=14.0.0"
},
"scripts": {
"test": "cross-env NODE_OPTIONS=--experimental-vm-modules jest --maxWorkers=4 --coverage",
"build": "node scripts/sync-version.js && rollup -c",
"typecheck": "tsc",
"lint": "eslint . && prettier --check .",
"fix": "eslint --fix . && prettier --write .",
"typecheck": "tsc",
"generate-bundles": "rollup -c",
"test-bundles": "yarn generate-bundles && node ./test/svgo.cjs && node ./test/browser.js",
"test-regression": "node ./test/regression-extract.js && cross-env NO_DIFF=1 node ./test/regression.js",
"prepublishOnly": "rimraf dist && yarn generate-bundles",
"qa": "yarn lint && yarn typecheck && yarn test && yarn test-bundles && yarn test-regression"
"test": "cross-env NODE_OPTIONS=--experimental-vm-modules jest --maxWorkers=4 --coverage",
"test:bundles": "yarn build && node ./test/svgo.cjs && node ./test/browser.js",
"test:regression": "node ./test/regression-extract.js && cross-env NO_DIFF=1 node ./test/regression.js",
"qa": "yarn typecheck && yarn lint && yarn test && yarn test:bundles && yarn test:regression",
"prepublishOnly": "rimraf dist && yarn build"
},
"jest": {
"coveragePathIgnorePatterns": [
Expand Down
12 changes: 12 additions & 0 deletions scripts/sync-version.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
import fs from 'node:fs/promises';
import path from 'path';
import { fileURLToPath } from 'url';

const __dirname = path.dirname(fileURLToPath(import.meta.url));
const pkgPath = path.join(__dirname, '../package.json');
const { version } = JSON.parse(await fs.readFile(pkgPath, 'utf-8'));

await fs.writeFile(
'./lib/version.js',
`/** Version of SVGO. */\nexport const VERSION = '${version}';\n`,
);
20 changes: 17 additions & 3 deletions test/browser.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,14 @@
import assert from 'assert';
import fs from 'node:fs/promises';
import http from 'http';
import path from 'path';
import { fileURLToPath } from 'url';
import { chromium } from 'playwright';

const __dirname = path.dirname(fileURLToPath(import.meta.url));
const pkgPath = path.join(__dirname, '../package.json');
const { version } = JSON.parse(await fs.readFile(pkgPath, 'utf-8'));

const fixture = `<svg xmlns="http://www.w3.org/2000/svg">
<g attr1="val1">
<g attr2="val2">
Expand All @@ -24,11 +30,12 @@ const expected = `<svg xmlns="http://www.w3.org/2000/svg">

const content = `
<script type="module">
import { optimize } from '/svgo.browser.js';
import { VERSION, optimize } from '/svgo.browser.js';
const result = optimize(${JSON.stringify(fixture)}, {
plugins : [],
js2svg : { pretty: true, indent: 2 }
});
globalThis.version = VERSION;
globalThis.result = result.data;
</script>
`;
Expand All @@ -50,8 +57,15 @@ const runTest = async () => {
const context = await browser.newContext();
const page = await context.newPage();
await page.goto('http://localhost:5000');
const actual = await page.evaluate(() => globalThis.result);
assert.equal(actual, expected);

const actual = await page.evaluate(() => ({
version: globalThis.version,
result: globalThis.result,
}));

assert.strictEqual(actual.version, version);
assert.equal(actual.result, expected);

await browser.close();
};

Expand Down
4 changes: 3 additions & 1 deletion test/svgo.cjs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
const { loadConfig, optimize } = require('../dist/svgo-node.cjs');
const assert = require('assert');
const { VERSION, optimize, loadConfig } = require('../dist/svgo-node.cjs');
const PKG = require('../package.json');

const fixture = `<svg xmlns="http://www.w3.org/2000/svg">
<g attr1="val1">
Expand Down Expand Up @@ -27,6 +28,7 @@ const runTest = () => {
});
const actual = result.data;

assert.strictEqual(VERSION, PKG.version);
assert.equal(actual, expected);
assert.notEqual(loadConfig, undefined);
};
Expand Down
12 changes: 9 additions & 3 deletions test/svgo/_index.test.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import fs from 'fs';
import fs from 'node:fs/promises';
import path from 'path';
import { EOL } from 'os';
import { fileURLToPath } from 'url';
import { optimize } from '../../lib/svgo.js';
import { VERSION, optimize } from '../../lib/svgo.js';

const __dirname = path.dirname(fileURLToPath(import.meta.url));

Expand All @@ -14,11 +14,17 @@ const normalize = (file) => {

const parseFixture = async (file) => {
const filepath = path.resolve(__dirname, file);
const content = await fs.promises.readFile(filepath, 'utf-8');
const content = await fs.readFile(filepath, 'utf-8');
return normalize(content).split(/\s*@@@\s*/);
};

describe('svgo', () => {
it('version should match package.json', async () => {
const pkgPath = path.resolve(__dirname, '../../package.json');
const { version } = JSON.parse(await fs.readFile(pkgPath, 'utf-8'));
expect(VERSION).toStrictEqual(version);
});

it('should create indent with 2 spaces', async () => {
const [original, expected] = await parseFixture('test.svg.txt');
const result = optimize(original, {
Expand Down