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

fix: export package.json and dist/svgo.browser.js again #1999

Closed
wants to merge 2 commits into from

Conversation

XhmikosR
Copy link
Contributor

@XhmikosR XhmikosR commented May 9, 2024

Fixes #1998

@XhmikosR XhmikosR changed the title fix: export package.json again fix: export package.json and dist/svgo.browser.js again May 9, 2024
@XhmikosR XhmikosR marked this pull request as ready for review May 9, 2024 05:09
@@ -59,10 +59,15 @@
"require": "./dist/svgo-node.cjs",
"types": "./lib/svgo-node.d.ts"
},
"./dist/svgo.browser.js": {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I'm not mistaken, that this will still fail if one requires dist/svgo.browser. We might need to tweak it.

@SethFalco
Copy link
Member

SethFalco commented May 9, 2024

Hmm, we could do something like this:

  "exports": {
    ".": {
      "import": "./lib/svgo-node.js",
      "require": "./dist/svgo-node.cjs",
      "types": "./lib/svgo-node.d.ts"
    },
    "./browser": {
      "import": "./dist/svgo.browser.js",
      "types": "./lib/svgo.d.ts"
    },
    "./dist/svgo.browser": {
      "import": "./dist/svgo.browser.js",
      "types": "./lib/svgo.d.ts"
    },
    "./bin/*": "./bin/*",
    "./dist/*": "./dist/*",
    "./lib/*": "./lib/*",
    "./plugins/*": "./plugins/*",
    "./package.json": "./package.json"
  },

We could also do:

  "exports": {
    ".": {
      "import": "./lib/svgo-node.js",
      "require": "./dist/svgo-node.cjs",
      "types": "./lib/svgo-node.d.ts"
    },
    "./browser": {
      "import": "./dist/svgo.browser.js",
      "types": "./lib/svgo.d.ts"
    },
    "./dist/svgo.browser": {
      "import": "./dist/svgo.browser.js",
      "types": "./lib/svgo.d.ts"
    },
    "./*": "./*"
  },

However, this will only make the imports available with the .js/.json suffix, excluding ./dist/svgo.browser which has an explicit alias defined. I think it's the best we could do for now without explicitly adding aliases for each and every file in the repo.

Alternatively, it could be worth deprecated v3.3.0 and v3.3.1, and making a new release v3.3.2 which reverts the changes. Then we make a v4.0.0 release, with less concern over breaking changes. (A lot of people are very keen to see removeViewBox disabled by default anyway, which was slotted for v4.)

I'm leaning on the latter, and will have a tag ready to push/release. However, I'll leave room for discussion before I do anything.

@XhmikosR
Copy link
Contributor Author

XhmikosR commented May 9, 2024

Agreed, @SethFalco the latter is the best choice.

But while at it, we should look into any other major changes slated for v4 like dropping any Node.js version support

@nuintun
Copy link
Contributor

nuintun commented May 9, 2024

The typesVersions field needs to be updated also.

Because exports.types does not work with typescript now.

"exports": {
  ".": {
    "import": "./lib/svgo-node.js",
    "require": "./dist/svgo-node.cjs",
    "types": "./lib/svgo-node.d.ts"
  },
  "./browser": {
    "import": "./dist/svgo.browser.js",
    "types": "./lib/svgo.d.ts"
  },
  "./dist/svgo.browser.js": {
    "import": "./dist/svgo.browser.js",
    "types": "./lib/svgo.d.ts"
  },
  "./*": "./*"
},
"typesVersions": {
  "*": {
    ".": [
      "./lib/svgo-node.d.ts"
    ],
    "browser": [
      "./lib/svgo.d.ts"
    ],
    "dist/svgo.browser.js": [
      "./lib/svgo.d.ts"
    ]
  }
}

@SethFalco
Copy link
Member

SethFalco commented May 9, 2024

Now that we're committed to do a v4 release instead, I'm thinking we could close this, and make the change in imports one of the documented breaking changes.

That way, there's no need to define the aliases and duplicate type versions.

I'll also have to look into ensuring that everything that users would want to import is still convenient to import, like individual plugins potentially.

If there are no qualms with that, feel free to close the issue. Otherwise, hit me with any feedback!

@XhmikosR
Copy link
Contributor Author

XhmikosR commented May 9, 2024

Sounds good!

Let's document the changes for the release notes

@XhmikosR XhmikosR closed this May 9, 2024
@XhmikosR
Copy link
Contributor Author

XhmikosR commented May 9, 2024

Actually, I still think we need package.json there, don't we @SethFalco ?

@SethFalco
Copy link
Member

Actually, I still think we need package.json there, don't we @SethFalco ?

As far as I'm concerned, package.json isn't considered part of our public interface, so normally I'd say it doesn't belong there.

I noticed in the other issue that it looked like you were importing it, though. Mind sharing your use case and we'll see if it makes sense, or maybe SVGO could do something better to export what is it you need?

For example, if you want the SVGO version at runtime, then this is something we could export ourselves.

@XhmikosR
Copy link
Contributor Author

XhmikosR commented May 9, 2024

We use it in svgomg for the version only during build time:

https://github.com/jakearchibald/svgomg/blob/d70f2619fbf6f662d04b6bccd9485558c06b52ba/gulpfile.js#L5

@SethFalco
Copy link
Member

SethFalco commented May 9, 2024

Understood, instead of exporting package.json then, how about we export a version like how some other libraries do:

import { version } from "react";
import _ from "lodash";
import axios from "axios";

console.log(version, _.VERSION, axios.VERSION);
// 18.3.1 4.17.21 1.6.8

I'll write up an issue later today, and can do the PR this weekend too.


Thanks for being so active with pull requests and issues, by the way. I'll be reviewing them over the next few days as I find time between other tasks.

This pull request was closed.
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.

package.json/ dist/svgo.browser.js are no longer exported
3 participants