VEuPathDB / web-components

Library of React components for plotting data
Apache License 2.0
1 stars 0 forks source link

update v3 and leaflet, fix a chartmarker bug #445

Closed moontrip closed 1 year ago

moontrip commented 1 year ago

I heard from Connor that react-leaflet-v3 is a high priority so I worked for this. This is branched out from react-leaflet-v3-ref to test several things:

a) update v3 b) update leaflet to the latest version, v1.9.3 c) fix a chartmarker bug (please see https://github.com/VEuPathDB/web-components/pull/444)

These updates seem to work fine with storybook as well as web-eda's mapViz. However, please note that somehow npm-pack-here at web-eda did not work natively. It throws an error:

An unexpected error occurred: "expected hoisted manifest for \"@veupathdb/components#react-leaflet\"".

I tried to find a solution and the one I could make it work is to explicitly specify react-leaflet v3 at dependenciesat web-eda's package.json, like:

  "dependencies": {
    "debounce-promise": "^3.1.2",
    "fp-ts": "^2.9.3",
    "io-ts": "^2.2.13",
    "lodash": "^4.17.21",
    "monocle-ts": "^2.3.11",
    "react-cool-dimensions": "^2.0.7",
    "react-draggable": "^4.4.3",
    **"react-leaflet": "^3.2.5",**
    "react-resizable": "^3.0.4",
    "uuid": "^8.3.2"
  },

IDK if there is another solution without specifying it. @dmfalke?

Anyway, I hope anyone can test this PR to make sure if everything works well.

p.s. Actually this branch can be directly merged into main as well, I suppose.

dmfalke commented 1 year ago

Thanks, @moontrip. I will test this out soon.

bobular commented 1 year ago

Hi @moontrip

Without fully understanding what's going on, I followed some googled advice on the "expected hoisted manifest" issue, and deleted/renamed yarn.lock and node_modules, then did yarn and yarn add -D file:local_modules/@veupathdb/components and yarn --check-files and it built without that error. :tada:

I'm not sure what this means going forward though. A PR on the web-eda side with these major changes to yarn.lock?

moontrip commented 1 year ago

@bobular wow, it sounds great 👍 I did try to remove yarn.lock but didn't try to delete node_modules.

bobular commented 1 year ago

But then it doesn't actually compile :sob:

There are lots of these very low-level looking React errors/warnings

webpack compiled with 24 warnings
ERROR in src/index.tsx:131:16
TS2786: 'DefaultComponent' cannot be used as a JSX component.
  Its element type 'ReactElement<any, any> | Component<Props, any, any> | null' is not a valid JSX element.
    Type 'Component<Props, any, any>' is not assignable to type 'Element | ElementClass | null'.
      Type 'Component<Props, any, any>' is not assignable to type 'ElementClass'.
        The types returned by 'render()' are incompatible between these types.
          Type 'React.ReactNode' is not assignable to type 'import("/home/maccallr/Desktop/EDA/web-eda/node_modules/@types/react-router/node_modules/@types/react/index").ReactNode'.
    129 |           >
    130 |             <SnackbarProvider styleProps={{}}>
  > 131 |               <DefaultComponent {...props} />
        |                ^^^^^^^^^^^^^^^^
    132 |             </SnackbarProvider>
    133 |           </UIThemeProvider>
    134 |         </DevLoginFormContext.Provider>
bobular commented 1 year ago

Probably due to mixed @types/react ?

$ yarn why @types/react | grep Found
=> Found "@types/react@16.14.35"
=> Found "@types/react-resizable#@types/react@18.0.27"
=> Found "@types/react-router-dom#@types/react@18.0.27"
=> Found "@types/react-table#@types/react@18.0.27"
=> Found "@testing-library/react-hooks#@types/react@18.0.27"
=> Found "@types/react-router#@types/react@18.0.27"
=> Found "@types/react-test-renderer#@types/react@18.0.27"
=> Found "@types/react-transition-group#@types/react@18.0.27"
=> Found "@visx/gradient#@types/react@18.0.27"
=> Found "@visx/group#@types/react@18.0.27"
...
bobular commented 1 year ago

I'm guessing there are some major React version issues here...

If I set the @types/react version to 18.0.27 (and a few others while I'm at it)

diff --git a/package.json b/package.json
index 2d10f0b1..f219d3ae 100644
--- a/package.json
+++ b/package.json
@@ -71,14 +71,14 @@
     "@types/lodash": "^4.14.182",
     "@types/luxon": "^3.0.1",
     "@types/node": "^12.0.0",
-    "@types/react": "^16.9.53",
-    "@types/react-dom": "^16.9.8",
-    "@types/react-resizable": "^1.7.2",
+    "@types/react": "^18.0.27",
+    "@types/react-dom": "^18.0.10",
+    "@types/react-resizable": "^1.7.4",
     "@types/react-router-dom": "^5.3.3",
-    "@types/react-table": "^7.7.12",
+    "@types/react-table": "^7.7.14",
     "@types/uuid": "^8.3.4",
     "@veupathdb/browserslist-config": "^1.1.0",
-    "@veupathdb/components": "^0.19.17",
+    "@veupathdb/components": "file:local_modules/@veupathdb/components",
     "@veupathdb/coreui": "^0.18.18",
     "@veupathdb/eslint-config": "^2.0.0",
     "@veupathdb/http-utils": "^1.1.0",

Then the compiler warnings change to something a bit different

webpack compiled with 24 warnings
ERROR in src/lib/core/__test__/hooks/useAnalysis.test.tsx:72:41
TS2339: Property 'children' does not exist on type '{}'.
    70 | } as AnalysisClient;
    71 |
  > 72 | const wrapper: React.ComponentType = ({ children }) => (
       |                                         ^^^^^^^^
    73 |   <WorkspaceContext.Provider
    74 |     value={{
    75 |       analysisClient,

Yarn is still installing react@17.0.2 so I doubt the 18.x types are going to work...

moontrip commented 1 year ago

@bobular well I didn't see such an error so I don't know what to say.

bobular commented 1 year ago

Hmm... so it worked for you?

I have gone down a rabbit hole with React version upgrades and that did NOT magically fix anything (no surprises!)

moontrip commented 1 year ago

@bobular well, I didn't try your method honestly as I am working on other stuff. As I said in my first description (https://github.com/VEuPathDB/web-components/pull/445#issue-1563388756), I could make it by manually adding react-leaflet at dependencies

bobular commented 1 year ago

@bobular well, I didn't try your method honestly as I am working on other stuff. As I said in my first description (#445 (comment)), I could make it by manually adding react-leaflet at dependencies

Sure - I was looking for a longer-term solution that we can merge to main - to save Dave some time.

I have found the following:

Forget everything about react-leaflet v3 and npm-pack-here etc.

If I delete yarn.lock and do a yarn again, we get the same compile warnings as in https://github.com/VEuPathDB/web-components/pull/445#issuecomment-1410633997

Something is causing @types/react to go to v18.0.27 (as in https://github.com/VEuPathDB/web-components/pull/445#issuecomment-1410642948) - yarn why says that a bunch of things are "asking" for that, for example @types/react-router-dom which has this dependency: "@types/react": "*",

Unfortunately, I think we're going to have to bother Dave to get a solution where we don't have to delete yarn.lock!

bobular commented 1 year ago

But maybe we don't need a permanent solution... Maybe it will work fine once the web-components module is merged and used via npm.

moontrip commented 1 year ago

I did some careful testing of the maps (FSM and viz) and thumbnails in EDA. All seems to be working fine.

My only comment is to be careful merging the mosaic changes (but maybe they are already in main - I notice that this isn't a PR to merge into main)

@bobular yes those are merged from main so they would not make a conflict. I think that it was because I didn't commit the merge of the main to react-leaflet-v3-ref. Now they are not shown.

moontrip commented 1 year ago

@bobular as you suggested at https://github.com/VEuPathDB/web-components/pull/444, I have changed the default color to silver, rgb(192,192,192) here too.

bobular commented 1 year ago

:tada: