search

hyperjump-io / json-schema

JSON Schema Validation, Annotation, and Bundling. Supports Draft 04, 06, 07, 2019-09, 2020-12, OpenAPI 3.0, and OpenAPI 3.1
https://json-schema.hyperjump.io/
MIT License
224 stars 21 forks source link

2a0ef5b and e200e30 were breaking changes and should have resulted in a major version increments #68

Closed cpcallen closed 4 months ago

cpcallen commented 4 months ago

A dependabot upgrade of @hyperjump/json-schema from v1.6.7 to v1.9.3 in our repo recently failed with the following error:

Error [ERR_MODULE_NOT_FOUND]: Cannot find package '@hyperjump/browser' imported from /home/runner/work/blockly/blockly/node_modules/@hyperjump/json-schema/lib/index.js
      at new NodeError (node:internal/errors:405:5)
      at packageResolve (node:internal/modules/esm/resolve:[91](https://github.com/google/blockly/actions/runs/9977034707/job/27570677318?pr=8363#step:7:93)6:9)
      at moduleResolve (node:internal/modules/esm/resolve:973:20)
      at defaultResolve (node:internal/modules/esm/resolve:1206:11)
      at ModuleLoader.defaultResolve (node:internal/modules/esm/loader:404:12)
      at ModuleLoader.resolve (node:internal/modules/esm/loader:373:25)
      at ModuleLoader.getModuleJob (node:internal/modules/esm/loader:250:38)
      at ModuleWrap.<anonymous> (node:internal/modules/esm/module_job:76:39)
      at link (node:internal/modules/esm/module_job:75:36) {
    code: 'ERR_MODULE_NOT_FOUND'
  }

It appears that the cause of this error is commit 2a0ef5b, which added many import … from '@hyperjump/browser' statements but only added a peerDependency on said package, which, in the presence of npm's legacy-peer-deps option is a breaking change.

I grant that not even the NPM maintainers themselves are in agreement about the exact meaning of "peer dependency" (vs. just plain "dependency"), but it's not clear to me that it's possible to use json-schema without also having browser installed, it seems reasonable that people might want to use json-schema without also using having any desire to use browser directly (such as everyone using json-schema prior to this refactor), and json-schema is not a plugin for browser, so this seems like a regular dependency to me.

If the factually-breaking nature of this commit is not sufficient to justify a major version bump, I would also note that there were also multiple API type changes (e.g. to Keyword.compile, introducing references to the Browser type, that would seem to have justified a major version bump on their own.

Having subsequently npm installed @hyperjump/browser as a (dev) dependency of our project, I then encountered the following error:

import {DETAILED} from '@hyperjump/json-schema/experimental';
        ^^^^^^^^
SyntaxError: The requested module '@hyperjump/json-schema/experimental' does not provide an export named 'DETAILED'
    at ModuleJob._instantiate (node:internal/modules/esm/module_job:123:21)
    at async ModuleJob.run (node:internal/modules/esm/module_job:191:5)
    at async ModuleLoader.import (node:internal/modules/esm/loader:336:24)
    at async loadESM (node:internal/process/esm_loader:34:7)
    at async handleMainPromise (node:internal/modules/run_main:106:12)

The proximate cause of this is e200e30, which removed the DETAILED export. Not sure what your policy is regarding the /experimental export, but broadly speaking if was your intention that features marked EXPERIMENTAL were subject to breaking changes without a major version bump then I think that was not adequately documented, and it would have been better from our point of view had this commit also resulted in a major version bump.

jdesrosiers commented 4 months ago

I'm sorry this ended up being a problem for you. I can't say that I'm 100% sure that I'm using peerDependency correctly. As you say, I'm not sure even NPM maintainers agree on what is correct. But, as I understand things, I think I can ague that I've done things correctly in this case.

I honestly wasn't aware of the legacy-peer-deps option, but my understanding is that by using it, you're opting in to managing peer dependencies manually rather than allowing npm to figure it out for you. So, when you get an error related a peer dependency not being installed, that's not the library's fault, it means you need to so some manual work that you opted in to do. So, I don't think that would constitute a breaking change. Also, it doesn't seem to me that the installation process is covered by the semver contract. Semver is about the behavior of the code.

there were also multiple API type changes (e.g. to Keyword.compile, introducing references to the Browser type, that would seem to have justified a major version bump

I'm not sure if you saw the section on versioning in the README. All the changes to the API are part of the Experimental API. The Stable API hasn't changed (other than being a lot faster).

json-schema is not a plugin for browser

Actually, it is. It's a lot more than that as well, but it's also a plugin for browser. It provides a plugin for the application/schema+json media type. It's also used by the application as a dependency to traverse JSON Schema documents, so it could be argued either way. I chose to make it a peer dependency because that results in the behavior I wanted. If users choose to use browser, json-schema should be extending the version of browser that they're using. If json-schema uses browser as a normal dependency, then users need to use the same version that json-schema uses. Otherwise npm installs two distinct versions of browser and the plugin doesn't get installed on the browser version the user is using. By making browser a peer dependency, json-schema can negotiate with the user installed browser and make sure only one version is installed and the plugin gets loaded correctly.

Hopefully, that helps explain why I made the choices I made. Again, I'm sorry this change caused friction for your process and I appreciate you taking the time to report it. However, I'm going to stand by my decision this time and keep things the way they are.