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

Add React types to fix build issue #2956

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

dn55533
Copy link
Contributor

@dn55533 dn55533 commented Dec 23, 2024

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:

  • @types/react
  • @types/react-dom

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

  • Changes to docs package checked and committed.
  • All acceptance criteria reviewed and met.

Testing

  • Relevant unit tests and visual regression tests added.
  • Visual testing against Figma component specification completed.
  • Playground stories in React Storybook up to date, with any prop changes and additions addressed.
  • Compare performance of modified components against develop using Performance addon in React Storybook.

Accessibility

  • Accessibility Insights FastPass performed.
  • A11y unit test added and yields no issues.
  • A11y plug-in on Storybook yields no issues.
  • Manual screen reader testing performed using NVDA and VoiceOver.
  • Manual keyboard testing for keyboard controls and logical focus order.
  • Correct roles used and ARIA attributes used correctly where required.
  • Logical heading structure is maintained, and the HTML elements used for headings can be changed to fit within the wider page structure.

Resize/zoom behaviour

  • Page can be zoomed to 400% with no loss of content.
  • Screen magnifier used with no issues.
  • Text resized to 200% with no loss of content.
  • Text spacing increased as per the WCAG 1.4.12 success criterion with no loss of content.

System modes

  • Browser setting 'prefers reduced motion' tested. No animations or motion visible whilst this setting is on.
  • Windows High Contrast mode tested with no loss of content.
  • System light and dark mode tested with no loss of content.
  • Browser support tested (Chrome, Safari, Firefox and Edge).

Testing content extremes

  • Min/max content examples tested with no loss of content or overflow.
  • All prop combinations work without issue.
  • Tested for FOUC (Flash of Unstyled Content) in both SSR (Server-Side Rendering) and SSG (Static Site Generation) settings.
  • Controlled and uncontrolled input components tested.
  • Props/slots can be updated after initial render.

@dn55533 dn55533 changed the title Add react types to fix build issue Add React types to fix build issue Dec 23, 2024
@ad9242
Copy link
Contributor

ad9242 commented Dec 30, 2024

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

npm install
npm run bootstrap
npm run build

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

@dn55533
Copy link
Contributor Author

dn55533 commented Dec 30, 2024

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.

@dn55533
Copy link
Contributor Author

dn55533 commented Dec 30, 2024

Just re-creacted the issue again using the following steps:

git clone https://github.com/mi6/ic-ui-kit.git
cd ic-ui-kit/
git checkout develop
git pull
npm install
npm run bootstrap
npm run build

Result:

@ukic/web-components:build

@ukic/web-components: > @ukic/[email protected] build
@ukic/web-components: > stencil build
@ukic/web-components: [40:28.3] @stencil/core
@ukic/web-components: [40:29.0] v4.23.0 
@ukic/web-components: [40:34.2] build, core, prod mode, started ...
@ukic/web-components: [40:34.3] transpile started ...
@ukic/web-components: [40:46.5] transpile finished in 12.21 s
@ukic/web-components: [ ERROR ] TypeScript: node_modules/@types/mdx/types.d.ts:23:38
@ukic/web-components: Cannot find namespace 'JSX'.
@ukic/web-components: L22: */
@ukic/web-components: L23: ype StringComponent = Extract<keyof JSX.IntrinsicElements, ElementTyp
@ukic/web-components: [40:46.5] build failed in 12.23 s

———————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————

Lerna (powered by Nx) Running target build for 2 projects failed

@ad9242
Copy link
Contributor

ad9242 commented Dec 30, 2024

Interestingly, where it shows the error, it seems to be using Stencil version 4.23.0
@ukic/web-components: [40:28.3] @stencil/core
@ukic/web-components: [40:29.0] v4.23.0 
@ukic/web-components: [40:34.2] build, core, prod mode, started ...

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?

Copy link
Contributor

@MI6-255 MI6-255 left a 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.

Copy link
Contributor

@GCHQ-Developer-112 GCHQ-Developer-112 left a 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

@dn55533 dn55533 force-pushed the fix/add-react-types-to-fix-build-issue branch from 7f90d94 to 19afb52 Compare January 6, 2025 10:20
@GCHQ-Developer-112
Copy link
Contributor

GCHQ-Developer-112 commented Jan 6, 2025

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

Few more thing, please refer to our CONTRIBUTING.md for guidance on how to format your commit. Please also set up verified/signed commits

@dn55533 dn55533 force-pushed the fix/add-react-types-to-fix-build-issue branch 5 times, most recently from 73da1f4 to 4df942e Compare January 6, 2025 11:33
@dn55533 dn55533 force-pushed the fix/add-react-types-to-fix-build-issue branch from 4df942e to 5243c4b Compare January 6, 2025 11:46
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.

4 participants