pod-browser icon indicating copy to clipboard operation
pod-browser copied to clipboard

Migrate to nextjs native server and lint, and more

Open ThisIsMissEm opened this issue 3 years ago • 9 comments

Description

Pod Browser had been using a custom http server for nextjs, which actually wasn't needed (strictly speaking) — the only use for it was to use SSL in development, which required installing or accepting a custom self-signed certificate for localhost, which is generally not a great idea.

Further, next now features a built in next lint command, which means you've less configuration to worry about (though linting standards changed a little).

The bigger change is that next dev now runs with SWC instead of babel — it's a "drop in" replacement, given the right configuration.

Unfortunately the tests via jest still require babel until we're able to upgrade jest, which is going to be reasonably tricky, but here's why it'll be really good to do this upgrade: https://nextjs.org/docs/testing#setting-up-jest-with-the-rust-compiler

I've also fixed a few outdated dependencies and removed unneeded dependencies.

This work also opened a bug fix on solid-client-authn-js, as we'd accidentally added a devDependency to dependencies (oops, it happens): https://github.com/inrupt/solid-client-authn-js/pull/1996

Changes

See above.

Testing

Commit checklist

  • [ ] All acceptance criteria are met.
  • [ ] Includes tests to ensure functionality and prevent regressions.
  • [ ] Relevant documentation, if any, has been written/updated.
  • [ ] The changelog has been updated, if applicable.

Interested parties

@daytonn

Notes

Screenshots/captures

ThisIsMissEm avatar Feb 27 '22 01:02 ThisIsMissEm

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/inrupt/pod-browser/Ea5mnSMmm67aygZxf9VY6gfcQ4Bu
✅ Preview: https://pod-browser-git-chore-upgrade-to-native-next-inrupt.vercel.app

vercel[bot] avatar Feb 27 '22 01:02 vercel[bot]

You may wish to check if you really need the SentryWebpackPlugin as it looks to be outdated or unmaintained (though I'm not familiar with it)

ThisIsMissEm avatar Feb 27 '22 01:02 ThisIsMissEm

Although worried about the error popping up in the automated CI output. FATAL ERROR: Ineffective mark-compacts near heap limit Allocation failed - JavaScript heap out of memory

solid-akb avatar Feb 28 '22 14:02 solid-akb

Although worried about the error popping up in the automated CI output. FATAL ERROR: Ineffective mark-compacts near heap limit Allocation failed - JavaScript heap out of memory

Hm, maybe we will need to increase the heap size then — might also be worth looking into the unit tests to see why they might be consuming excessive memory (e.g., is there a memory leak somewhere?)

ThisIsMissEm avatar Mar 05 '22 18:03 ThisIsMissEm

Okay, just did a quick test to see just how bad the memory leaking is, and it seems to be on the order of 20-100mb per test file: Screenshot 2022-03-05 at 20 16 43

After getting a memory profile out of the chrome developer tools, it seems to be largely material-ui related. We might wish to try to upgrade to MUI v4.12, as we're currently using the older 4.11, and there could be performance improvements.

Alternatively, we could upgrade to MUI v5, and there's a guide here on doing that: https://mui.com/guides/migration-v4/

More information on MUI memory leaks: https://github.com/mui/material-ui/issues/21528

Screenshot 2022-03-05 at 20 18 30

There is a codemod available for migrating up, which would hopefully automatically upgrade us from useStyles, which might help: https://mui.com/guides/migration-v4/#1-use-styled-or-sx-api

Also, somewhat related to this PR, currently the project depends on whatwg-fetch for mocking responses in tests, migrating to jest-fetch-mock may be very advantageous: https://www.npmjs.com/package/jest-fetch-mock

There's a guide here: https://www.leighhalliday.com/mock-fetch-jest

(cc @NSeydoux: we may wish to use mock-fetch-jest in the SDKs too!)

ThisIsMissEm avatar Mar 05 '22 19:03 ThisIsMissEm

With cross-site security warnings, we initially required SSL in dev (even if it's self-signed.) Is that no longer an issue?

ajacksified avatar Mar 18 '22 18:03 ajacksified

I can run the server and the tests, but I'm getting an error when I click the "Sign in" button so I'm not sure how to test the application.

Error:

{"error":"invalid_grant","error_description":"Invalid redirect_uri"}

Is this to what you were referring @ajacksified?

daytonn avatar Mar 18 '22 22:03 daytonn

With cross-site security warnings, we initially required SSL in dev (even if it's self-signed.) Is that no longer an issue?

It worked fine when I tested it, I think, it may require updating that json file in the appsteam pod to include http://localhost:3000 as well.

What dayton saw was probably that (the client ID mismatching because we're switching from https to http).

I don't think cross-site security warnings should be an issue http://localhost:PORT to https://something has always worked as far as I know, just doing https -> http resources resulted in a mixed content warning or error.

Maybe at some level of auditing for security issues you might face problems if running that against localhost, but that's not representative of a production deployment. Would be good to know more on what you're referring to @ajacksified

ThisIsMissEm avatar Mar 19 '22 00:03 ThisIsMissEm

I've also been able to migrate to using @swc/jest which would allow dropping all dependency on babel here: https://github.com/inrupt/pod-browser/compare/experiment/migrate-to-swc-jest, specifically in https://github.com/inrupt/pod-browser/commit/be299f4d728b404a4aa7db588e9919c26335013c

ThisIsMissEm avatar Apr 10 '22 20:04 ThisIsMissEm