microbiomedata / nmdc-field-notes

Mobile app for metadata collection on the go
https://fieldnotes.microbiomedata.org/
Other
1 stars 0 forks source link

upgrade to capacitor 6 #109

Closed yxu-lanl closed 1 month ago

yxu-lanl commented 1 month ago

Migrate capacitor 5 to 6 Capacitor 6 sets a deployment target of iOS 13 and Android 14 (SDK 34)

  1. update capacitor to 6 https://capacitorjs.com/docs/updating/6-0 Using the CLI to Migrate npm i -D @capacitor/cli@latest npx cap migrate

  2. Update plugins to 6 https://capacitorjs.com/docs/updating/plugins/6-0 Using @capacitor/plugin-migration-v5-to-v6 npx @capacitor/plugin-migration-v5-to-v6@latest

  3. Sync (copy + update) an Ionic project ionic cap sync

yxu-lanl commented 1 month ago

@pkalita-lbl I couldn't figure out why the unit test failed. There is no any change inside the src directory.

pkalita-lbl commented 1 month ago

It looks like the failure is happening when running npm run build, before it even gets to the unit tests. Do you see the error locally when running npm run build?

yxu-lanl commented 1 month ago

No.

> nmdc-field-notes@0.0.1 build
> tsc && vite build

vite v5.2.11 building for production...
✓ 823 modules transformed.
dist/assets/web-legacy-C7W-XhZu.js                 0.46 kB │ gzip:   0.30 kB
dist/assets/status-tap-legacy-7fB9rwAc.js          0.60 kB │ gzip:   0.40 kB
dist/assets/web-legacy-D1jaVo-u.js                 0.76 kB │ gzip:   0.39 kB
dist/assets/swipe-back-legacy-puD0-1n1.js          0.78 kB │ gzip:   0.52 kB
dist/assets/web-legacy-ZgV_DYjn.js                 0.99 kB │ gzip:   0.51 kB
dist/assets/focus-visible-legacy-CdO5cX4I.js       1.09 kB │ gzip:   0.55 kB
dist/assets/md.transition-legacy-iFp7HxjL.js       1.13 kB │ gzip:   0.61 kB
dist/assets/index9-legacy-CyCUJo2M.js              1.74 kB │ gzip:   0.87 kB
dist/assets/input-shims-legacy-Dy5TMB-4.js         4.73 kB │ gzip:   2.08 kB
dist/assets/ios.transition-legacy-CkAsW4xI.js     10.64 kB │ gzip:   3.03 kB
dist/assets/polyfills-legacy-Dw99Pvtb.js          68.93 kB │ gzip:  27.38 kB
dist/assets/index-legacy-BVVo-Rpu.js           1,261.13 kB │ gzip: 304.75 kB

(!) Some chunks are larger than 500 kB after minification. Consider:
- Using dynamic import() to code-split the application
- Use build.rollupOptions.output.manualChunks to improve chunking: https://rollupjs.org/configuration-options/#output-manualchunks
- Adjust chunk size limit for this warning via build.chunkSizeWarningLimit.
dist/index.html                                  4.13 kB │ gzip:   1.71 kB
dist/assets/logo-text-DQe-ClQo.svg               6.52 kB │ gzip:   1.86 kB
dist/assets/NMDC_microbe_light-CnrD6x0E.png    538.52 kB
dist/assets/NMDC_microbe_dark-BdaioxqE.png     539.26 kB
dist/assets/index-CyW8DWhf.css                  33.37 kB │ gzip:   6.37 kB
dist/assets/web-CNtCtqRu.js                      0.38 kB │ gzip:   0.25 kB
dist/assets/status-tap-B3Kgygg_.js               0.48 kB │ gzip:   0.34 kB
dist/assets/web-BrTeqAOd.js                      0.67 kB │ gzip:   0.33 kB
dist/assets/swipe-back-Dgz2aUDn.js               0.68 kB │ gzip:   0.47 kB
dist/assets/web-Ufj_jgVm.js                      0.91 kB │ gzip:   0.46 kB
dist/assets/focus-visible-supuXXMI.js            0.99 kB │ gzip:   0.51 kB
dist/assets/md.transition-kWh3BLAl.js            1.07 kB │ gzip:   0.57 kB
dist/assets/index9-B7lzdqak.js                   1.63 kB │ gzip:   0.83 kB
dist/assets/input-shims-Dy297fLT.js              4.82 kB │ gzip:   2.08 kB
dist/assets/ios.transition-DPdIA-8W.js          11.21 kB │ gzip:   3.10 kB
dist/assets/index-2qqnZN4y.js                1,204.44 kB │ gzip: 302.25 kB

(!) Some chunks are larger than 500 kB after minification. Consider:
- Using dynamic import() to code-split the application
- Use build.rollupOptions.output.manualChunks to improve chunking: https://rollupjs.org/configuration-options/#output-manualchunks
- Adjust chunk size limit for this warning via build.chunkSizeWarningLimit.
✓ built in 14.74s
yxu-lanl commented 1 month ago

But 'npm run test.unit' failed.

FAIL  src/queries.test.tsx > useSubmission should replace paused mutations when offline
AssertionError: expected 'TEST 1' to be 'UPDATE 2' // Object.is equality

- Expected
+ Received

- UPDATE 2
+ TEST 1

 ❯ src/queries.test.tsx:191:5
    189|   expect(
    190|     result.current.query.data?.metadata_submission.studyForm.studyName,
    191|   ).toBe("UPDATE 2");
       |     ^
    192| });
    193| 

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[1/1]⎯

 Test Files  1 failed | 9 passed (10)
      Tests  1 failed | 62 passed (63)
eecavanna commented 1 month ago

Thanks for working on this, @yxu-lanl. I'll take a look at these errors now, too.


Notes

$ node --version
v20.8.1

$ npm install
$ npm run test.unit

I reproduced the error locally:

 FAIL  src/queries.test.tsx > useSubmission should replace paused mutations when offline
AssertionError: expected 'TEST 1' to be 'UPDATE 2' // Object.is equality

- Expected
+ Received

- UPDATE 2
+ TEST 1

 ❯ src/queries.test.tsx:191:5
    189|   expect(
    190|     result.current.query.data?.metadata_submission.studyForm.studyName,
    191|   ).toBe("UPDATE 2");
       |     ^
    192| });
    193| 

The failing test is defined here: https://github.com/microbiomedata/nmdc-field-notes/blob/d1a22d2dc76e2e62f0da79e864a9e764fe7c5122/src/queries.test.tsx#L143C7-L192

I wonder whether it uses something that is dependent upon the version of Capacitor or one of its plugins.

The expect(patchCount).toBe(1); statement (which comes before the failing one) is not failing; so I know that patchCount is 1 at that point. However, the value in the studyName field (in the failing expect() statement) is what I would expect if 0 patches had been applied, since the value matches the original value (as opposed to matching either of the patch values).

I'll defer to @pkalita-lbl on this as he wrote the original test (which is passing on the tip of the main branch). If it remains unresolved in a few days, I'll look at it more at that time.

eecavanna commented 1 month ago

I, too, am able to $ npm run build this branch locally with no issues. I am using $ node --version v20.8.1 on an M1 Mac. I think the GitHub Actions runner is an x86/x64 Linux machine.

eecavanna commented 1 month ago

I just looked at the GitHub Actions log and saw that the error was:

image

When I first checked out this branch, I also got an error (I think it was the same error) when I ran $ npm install locally. I ended up deleting package-lock.json and the entire node_modules folder and re-running $ npm install, at which point it ran OK.

I'll look into how a similar "workaround" can be applied to the GitHub Actions runner.

eecavanna commented 1 month ago

Possibly related to the build error: https://github.com/vitejs/vite/discussions/15532#discussioncomment-8141236 — the workaround ("patch") described there is to add the following object to package.json and then run $ npm install:

  "optionalDependencies": {
    "@rollup/rollup-linux-x64-gnu": "4.9.5"
  }

Edit: I confirmed that this works around the build error.

pkalita-lbl commented 1 month ago

Sorry I've been occupied with other tasks. It looks like the build error has been resolved, but there's still a test failure. I can try to take a look and see if I can tell what's going on.

pkalita-lbl commented 1 month ago

Looking at the diff between what npm list shows in main versus this branch I see, in part:

< ├── @tanstack/eslint-plugin-query@5.18.0
< ├── @tanstack/query-async-storage-persister@5.18.0
< ├── @tanstack/react-query-devtools@5.18.0
< ├── @tanstack/react-query-persist-client@5.18.0
< ├── @tanstack/react-query@5.18.0
---
> ├── @tanstack/eslint-plugin-query@5.32.1
> ├── @tanstack/query-async-storage-persister@5.35.1
> ├── @tanstack/react-query-devtools@5.35.1
> ├── @tanstack/react-query-persist-client@5.35.1
> ├── @tanstack/react-query@5.35.1

So it looks like the changes to package-lock.json in this branch increment the locked versions of more than just the Capacitor dependencies. I don't know if that was intentional or not. But I'm almost certain that some change introduced between versions 5.18 and 5.35 of the TanStack query dependencies is what's causing the test failure. It's something that we should definitely investigate some time (maybe we could bisect it down to a specific version and go from there). But if we don't need to change the locked version of those dependencies now, let's not.

yxu-lanl commented 1 month ago

Changed all 'node_modules/@tanstack' versions to '5.18.0' inside the package-lock.json. The unit test issue got fixed.