Skip to content
This repository was archived by the owner on Sep 23, 2025. It is now read-only.

Conversation

ThisIsMissEm
Copy link
Contributor

@ThisIsMissEm ThisIsMissEm commented Feb 27, 2022

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): inrupt/solid-client-authn-js#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

@vercel
Copy link

vercel bot commented Feb 27, 2022

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

@ThisIsMissEm
Copy link
Contributor Author

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)

@@ -0,0 +1,12 @@
{
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For some reason next lint didn't pick up the .js configuration, but we don't actually need it anyway.

test("it renders a bookmark icon", async () => {
const { asFragment } = renderWithTheme(
<BookmarkContext.Provider value={(bookmarks, setBookmarks)}>
<BookmarkContext.Provider value={{ bookmarks, setBookmarks }}>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@VirginiaBalseiro this may have been causing test bugs

});
});

describe("setupErrorComponent", () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test case is really testing internals of the AppAvatar component — it'd be better to find a way to inject an error state into the component. Additionally, it's worth looking into whether errorComponent can just be defined as a stand-alone component, rather than a component factory.

"@typescript-eslint/parser": "^5.11.0",
"@typescript-eslint/eslint-plugin": "^5.12.1",
"@typescript-eslint/parser": "^5.12.1",
"babel-eslint": "^10.1.0",
Copy link
Contributor Author

@ThisIsMissEm ThisIsMissEm Feb 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it may be possible to remove babel-eslint, iirc, swc will lint anyway, and you've a lint check on pre-push and CI, so you're not likely to land code that's not linted, and your editor should hopefully complain if you've linting issues

Copy link
Contributor

@solid-akb solid-akb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would recommend someone else take a look but everything looks great! Nice!

@solid-akb
Copy link
Contributor

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

@ThisIsMissEm
Copy link
Contributor Author

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
Copy link
Contributor Author

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: mui/material-ui#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!)

next lint is the preconfigured eslint from nextjs
This allows you to benefit from using SWC which is quicker than babel for the development server
We shouldn't need to set these parameters as Jest should automatically detect the correct number of workers for the environment, and not exceed the normal heap size
@ThisIsMissEm ThisIsMissEm force-pushed the chore/upgrade-to-native-next branch from 102aacc to 46fd036 Compare March 17, 2022 18:14
@ThisIsMissEm ThisIsMissEm marked this pull request as ready for review March 17, 2022 18:19
@ajacksified
Copy link
Contributor

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

@daytonn
Copy link

daytonn commented Mar 18, 2022

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?

@ThisIsMissEm
Copy link
Contributor Author

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
Copy link
Contributor Author

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 be299f4

Copy link
Contributor

@solid-akb solid-akb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good - let's merge and keep an eye on it (not expecting anything, but hard to test explicitly)

@ThisIsMissEm ThisIsMissEm marked this pull request as draft May 26, 2022 17:30
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants