rcpch / digital-growth-charts-react-component-library

A typescript React library for displaying RCPCH Digital Growth Charts from API data
https://growth.rcpch.ac.uk
MIT License
7 stars 11 forks source link

Build Issues with NPM package - Missing icon + no default export #34

Closed dmc-cambric closed 2 years ago

dmc-cambric commented 2 years ago

Hi Team,

Having the following build issues when installing the charts component library with npm:

Error 1: node_modules/@rcpch/digital-growth-charts-react-component-library/build/RCPCHChart/RCPCHChart.d.ts(1,8): error TS1192: Module '"C:/source/Morse/Client/node_modules/@types/react/index"' has no default export.

Error 2: C:\source\Morse\Client\node_modules/@rcpch/digital-growth-charts-react-component-library/build/index.d.ts(2,18): error TS2307: Cannot find module '../src/images/icon.png'.

Regarding issue 1, I can resolve by manually editing the ‘RCPCHChart.d.ts’ file and replacing import React from 'react'; with import * as React from 'react';

For issue 2, the icon doesn't seem to be present in the ‘node_modules\@rcpch\digital-growth-charts-react-component-library’ folder although I can see it in the RCPCH github repo here. Locally resolved by commenting out import and export of icon in ‘index.d.ts’ file.

I'm using: "react": "17.0.2", "typescript": "3.2.4" "@rcpch/digital-growth-charts-react-component-library": 6.1.4
"@types/react": "17.0.2"

Any help would be much appreciated!

eatyourpeas commented 2 years ago

Thanks Daniel - I am grateful for you pointing this out. I might be barking up the wrong tree but this feels like it might be to do with package versions. The typescript package version in the charts is ^3.7.2. There were some changes made in Typescript 3.6 around this issue which makes me wonder whether you have an incompatibility using 3.2.4? Are you able to change your version to test this idea?

dmc-cambric commented 2 years ago

Thanks for quick reply, I've experimented using both typescript 3.6 and 3.7 which did resolve the import issue. Unfortunately, the solution I'm implementing this on is dependent on typescript 3.2 .

However, I have found a work-around for this older version that has resolved it, which was adding the following to tsconfig.json: "esModuleInterop": true

In all TS versions tried, the error around the icon import still persists, any ideas?

eatyourpeas commented 2 years ago

"esModuleInterop": true is already set in the charting component, so are you referring to the tsconfig.json in your project? The rcpch charting component is a typescript project that uses Rollup (rather than webpack) as its bundler, and has a plug-in called @rollup/plugin-image (found in rollup.config.js) which allows small images to be exported as default const, an ES6 feature. I don't think I am clear why it is not working for you. I wonder if @chvanlennep has some suggestions.

As for typescript version clashing - this is I think a well-recognised vulnerability of the typescript ecosystem that microsoft have yet to fix. I am not sure of a work around that does not involve headaches. If you are happy to carry on the conversation about this back on email, as that is perhaps separate to this.

chvanlennep commented 2 years ago

Re TS1192, I think the issue here @dmc-cambric is that @types/react in the chart library are pre 17, versus the types in your project are 17. Importing React via default import is actually discouraged now, but has been a very common practice from the very beginning. Even though default export still works for react, the team have indicated that this will probably be deprecated in the future. @dmc-cambric the latest react types do come with several issues regarding backwards compatibility with other libraries you may end up using (namely the typing of React Children), so just be aware of that!

Since React 17, they have actually introduced an automatic JSX transform by default, which means that it is no longer necessary to import React in files with JSX. @eatyourpeas - it may require a project config tweak but you may just be able to remove the React imports. If that turns out to be too faffy, changing to a namespace import import * as React from 'react'; would be future proof.

Re TS2307, that one is a bit more difficult to be sure.. my guess would be your tsconfig @dmc-cambric needs a tweak to its module resolution strategy, but I am not sure. This may help.

eatyourpeas commented 2 years ago

thanks @chvanlennep for top advice - refactored as suggested.