-
Notifications
You must be signed in to change notification settings - Fork 33
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
Add React types to fix build issue #2956
base: develop
Are you sure you want to change the base?
Add React types to fix build issue #2956
Conversation
It shouldn't need React specific packages adding at the root level. If anywhere, we'd expect them to be needed in react & canary-react packages (in theory only for running storybook or the cypress tests, where the react components are used) What was the reason for adding them at this level? Doing the following in the root folder
does not seem to cause any issues, so it would be good to get to the bottom of if this is something environmental before adding additional dependencies for everyone to the root level Also, if they were to be added, ideally they would not need to be locked to a specific version and could be locked to a minimum version as with all other dependencies |
Without these dependencies, the project fails to build using those three commands. I tried multiple times, including cloning the repo from scratch into a new folder. When I added those dependencies, it fixed the problem. I'm not sure where the best place to put these dependencies, so I went with the root level. Happy to move them if needed. |
Just re-creacted the issue again using the following steps: git clone https://github.com/mi6/ic-ui-kit.git Result:
@ukic/web-components: > @ukic/[email protected] build ———————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————
|
Interestingly, where it shows the error, it seems to be using Stencil version 4.23.0 When we follow the steps you mention, it installs Stencil v4.10.0 as set in the package-lock.json Do you know of anything in your setup that could be causing the newer version of Stencil to be installed, as we are struggling to recreate this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently waiting on a discussion internally, marking as requested changes to prevent a merge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @dn55533, I've been able to recreate the issue you were facing. Please can you install the two types packages at version 17 so they match the versions of the react
and react-dom
dev dependencies that are already installed. Please can you also do the install in the web-components
folder instead of the root folder
7f90d94
to
19afb52
Compare
Few more thing, please refer to our CONTRIBUTING.md for guidance on how to format your commit. Please also set up verified/signed commits |
65d32e1
to
a7135b5
Compare
73da1f4
to
4df942e
Compare
4df942e
to
5243c4b
Compare
Summary of the changes
Fixed build problem in web-components by adding dependency on React types.
The npm run build task was failing with the following error: Cannot find namespace 'JSX'
This has been fixed by adding dev dependencies on the following packages:
The versions of these dependencies being used ("^17.0.37") match those being used in the canary-react package.
Related issue
N/A
Checklist
General
Testing
Accessibility
Resize/zoom behaviour
System modes
Testing content extremes