pod-browser
pod-browser copied to clipboard
Migrate to nextjs native server and lint, and more
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
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
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)
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
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?)
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:

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
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!)
With cross-site security warnings, we initially required SSL in dev (even if it's self-signed.) Is that no longer an issue?
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?
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
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