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

feat: export version constant #2016

merged 1 commit into from
May 27, 2024

Conversation

SethFalco
Copy link
Member

@SethFalco SethFalco commented May 26, 2024

Adds a VERSION export, similar to React, Axios, and Lodash.

This will enable third-party libraries to check what version of SVGO is in use during runtime, and is intended to supersede the need to import our package.json file, which we don't want to make part of our public API.

Chores

Updates our ESLint config from 2021 to 'latest'. I wanted to keep it at 2022 or later as we can use top-level awaits.

Usage

import { VERSION } from 'svgo';

console.log(VERSION);
// 4.0.0
const svgo = require('svgo');

console.log(svgo.VERSION);
// 4.0.0

Related

@@ -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 :)

@SethFalco SethFalco merged commit 78403d3 into svg:main May 27, 2024
10 checks passed
@SethFalco SethFalco deleted the version branch May 27, 2024 19:28
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

Successfully merging this pull request may close these issues.

2 participants