iTwin / viewer

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

iModel viewer install script error #318

Open Johannes-repos opened 1 week ago

Johannes-repos commented 1 week ago

having issues with the provided iModel viewer install script at https://developer.bentley.com/tutorials/web-application-quick-start/

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

After installing and running npm run start, it throws a bunch of errors. Looks like a ton of breaking changes and missing modules,

Did something change in the last few days? I used maybe a week or two ago and it worked fine.

2024-06-26_17-03-23

"@itwin/property-grid-react": "^1.9.0",
aruniverse commented 1 week ago

wonder if this is the cause: https://github.com/iTwin/iTwinUI/pull/2102

Johannes-repos commented 1 week ago

I managed to reduce the number of errors by adding the overrides and dependencies to the package.json but still get some errors. 2024-06-27_09-42-27

DanRod1999 commented 1 week ago

updating these packages to latest appear to have fixed the problem for me "@itwin/itwinui-react": "^3.12.0", "@types/react": "^18.3.3", "@types/react-dom": "^18.3.0", "react": "^18.3.1", "react-dom": "^18.3.1",

I do not know if latest is necessary, but this removed all errors and I was able to get my viewer running as normal, however I'm still receiving this warning

WARNING in ./node_modules/@itwin/itwinui-react/esm/core/Overlay/Overlay.js 47:14-32 Critical dependency: the request of a dependency is an expression @ ./node_modules/@itwin/itwinui-react/esm/index.js 66:0-52 66:0-52 @ ./src/App.tsx 13:0-54 107:39-53 @ ./src/index.tsx 12:0-24 39:35-38

I will continue to look into this, but hopefully this gets you going

mayank99 commented 1 week ago

It looks like this might be an issue in React 17 not correctly supporting ESM.

React 17 does not specify an "exports" field (see package.json), so react/jsx-runtime does not make sense in an ESM environment.

In contrast, React 18 does specify "exports" (see package.json, line 28).

Edit: I did some digging and found that the React team was made aware of this issue back in 2020, and they willingly decided not to fix it until v18.

The fix here would be to use React 18, or patch React 17 to add the missing "exports" field. Alternatively, you could add a webpack alias to make react/jsx-runtime resolve to react/jsx-runtime.js.


The "Critical dependency" warning is unrelated and can be safely ignored. We load a polyfill for the inert attribute from a CDN, but only in older, unsupported browsers.

aruniverse commented 1 week ago

Considering most bentley apps are still on react 17, what can we do in itwinui to not break those apps?

mayank99 commented 1 week ago

I don't think we can do anything tbh. The new JSX transform is required for React 19, which we are currently working to add support for in the next minor release.

It's worth noting that the original React announcement clearly mentions that the new JSX transform is supposed to work in older versions of React, so we haven't broken any semver rules in iTwinUI by switching to this transform in a minor release.

I know this situation is not ideal, but I sorta view this as a good thing. Applications should not be using such old React versions. React 18 was released more than two years ago, which is plenty of time to upgrade, especially considering that there were no large breaking changes. Additionally, React 18 adds some very important performance-oriented features which we'd like to take advantage of in iTwinUI (with no-op shims for React 17).

Going back to the original issue, it seems like the CRA template could be easily updated to use React 18?

aruniverse commented 1 week ago

Going back to the original issue, it seems like the CRA template could be easily updated to use React 18?

Yea we can easily resolve the original issue, but was looking for a solution for the larger itwinjs / bentley applications.

mayank99 commented 1 week ago

It's also worth mentioning that applications that are using CommonJS or partial ESM (e.g. webpack 4) are likely unaffected.

For affected applications, I would probably advise them to use one of the workarounds I mentioned earlier (i.e. patching or adding aliases).