iTwin / viewer

Monorepo that contains the iTwin Viewer npm packages and their related packages
MIT License
24 stars 15 forks source link

Default packages used by visualizer do not follow semantic versioning #257

Closed Michael-Tajmajer-Bentley closed 10 months ago

Michael-Tajmajer-Bentley commented 1 year ago

Followed instructions at: https://developer.bentley.com/tutorials/web-application-quick-start/

  1. Executed: npx create-react-app your-app-name --template @itwin/web-viewer --scripts-version @bentley/react-scripts

  2. Configured client id, etc.

  3. Update any of the following packages to a new minor version (which should not include breaking changes)

@itwin/browser-authorization 0.5.1 → 0.8.0 causes build to fail due to breaking change:

Attempted import error: 'BrowserAuthorizationCallbackHandler' is not exported from '@itwin/browser-authorization' (imported as 'BrowserAuthorizationCallbackHandler').

TS2305: Module '"@itwin/browser-authorization"' has no exported member 'BrowserAuthorizationCallbackHandler'. 7 | import "./index.scss"; 8 |

9 | import { BrowserAuthorizationCallbackHandler } from "@itwin/browser-authorization"; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 10 | import React from "react"; 11 | import ReactDOM from "react-dom"; 12 |

@itwin/property-grid-react 0.5.5 → 0.9.0 causes npm install to fail due to new dependency on iTwin v4 package (a -dev dependency!)

npm ERR! code ETARGET npm ERR! notarget No matching version found for @itwin/appui-react@^4.0.0-dev.13. npm ERR! notarget In most cases you or one of your dependencies are requesting npm ERR! notarget a package version that doesn't exist.

@itwin/tree-widget-react 0.4.7 → 0.8.0 causes npm install to fail due to new dependency on iTwin v4 package (a -dev dependency!)

npm ERR! code ETARGET npm ERR! notarget No matching version found for @itwin/appui-react@^4.0.0-dev.13. npm ERR! notarget In most cases you or one of your dependencies are requesting npm ERR! notarget a package version that doesn't exist.

Changes to minor version should not break applications: MINOR version when you add functionality in a backwards compatible manner - see: https://semver.org/

Changing dependencies to point to the next major version must have a major version change as well, going from a 0.5.5 to a 0.9.0 should not do this.

Finally, referring to a -dev version, which is not available, is a bad practice.

aruniverse commented 1 year ago

Update any of the following packages to a new minor version (which should not include breaking changes)

Please see semver spec. Semver for 0.x doesn't apply like you normally think it would. Breaking changes are allowed at any time. Use at your own risk.

Michael-Tajmajer-Bentley commented 1 year ago

Thanks for pointing that out - though I am still cranky about this because:

  1. Following the public instructions results in non-buildable code - perhaps we should not make these packages available until they are not so fragile? This is a huge turn-off to developers.

  2. Referencing packages that are not available (@itwin/appui-react@^4.0.0-dev.13) from publicly available packages leaves a bad impression; that we are not a professional organization.

These types of issues make me (as a third-party developer) hesitate before I invest in building something on top of the Bentley platform. At a minimum, there should be documentation on the site that explains these constraints.

aruniverse commented 10 months ago

You bring up valid concerns Mike. We were running into issues publishing the vcr pkgs (tree-widget, property-grid, measure-tools) as pre-releases while working through the itwinjs-core 4.x upgrade. That has since been resolved.

The viewer templates only reference one last 0.x pkg (measure-tools-react) due to other reasons. And the the desktop viewer pkg and desktop template references a 0.x ver of electron-authorization, but we are actively working towards updating that to 1.x, and we realize any changes for that will be breaking and it will be reflected as such.

This issue stems from growing pains of a new library/ecosystem we are developing. Closing this issue as the main concerns have been addressed and we have a plan moving fwd to handle this.