Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add SSR testing playground setup #455

Merged
merged 6 commits into from Feb 17, 2021
Merged

Add SSR testing playground setup #455

merged 6 commits into from Feb 17, 2021

Conversation

benoitgrelard
Copy link
Collaborator

@benoitgrelard benoitgrelard commented Feb 12, 2021

This PR:

  • update our yarn version to latest as it seem to be a blocker when installing deps in setting up a nextjs project
  • adds a new workspace called ssr-testing with a nextjs project
  • sets up nextjs so that it picks up our local packages sources instead of builds

NOTE: If I'm completely honest, I don't fully understand how the with-transpiled-modules thing work, considering I'm not giving it any modules in the array, but it works… 🤷‍♂️

Comment on lines +1 to +19
const path = require('path');
const withTM = require('next-transpile-modules')([]);

module.exports = withTM({
webpack(config) {
// https://github.com/josephluck/next-typescript-monorepo/blob/master/blog/next.config.js
const babelRule = config.module.rules.find((rule) =>
rule.use && Array.isArray(rule.use)
? rule.use.find((u) => u.loader === 'next-babel-loader')
: rule.use.loader === 'next-babel-loader'
);

if (babelRule) {
babelRule.include.push(path.resolve('../'));
}

return config;
},
});
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This took me ages to figure out, it works but I don't really get how…

Copy link
Contributor

Choose a reason for hiding this comment

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

well if it works, I guess that's all we need haha. It's just an internal thing anyway 🤷‍♀️

@@ -0,0 +1,18 @@
import * as React from 'react';
import { RadioGroup, RadioGroupItem, RadioGroupIndicator } from '@radix-ui/react-radio-group';
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We're able to import the packages like this, and they import the source and everything rebuilds and hot reloads when making changes.

@@ -0,0 +1,13 @@
{
"extends": "../tsconfig.json",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Extending our base config to get the path mapping

@benoitgrelard benoitgrelard marked this pull request as ready for review February 12, 2021 21:44
@benoitgrelard
Copy link
Collaborator Author

benoitgrelard commented Feb 12, 2021

awww man 😢 now getting this build error here in CI…

image

Copy link
Contributor

@jjenzz jjenzz left a comment

Choose a reason for hiding this comment

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

Other than the build error and the following, looks good 👍

Obviously there is the id inconsistency in RadioGroup but we'll fix that separately I presume!

ssr-testing/pages/index.tsx Outdated Show resolved Hide resolved
@@ -31,7 +32,7 @@
},
"devDependencies": {
"@babel/core": "^7.0.0",
"@stitches/react": "canary",
"@stitches/react": "0.0.3-canary.4",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I had to fix this to latest alpha because they've been publishing new betas and as I've busted the yarn lock we were getting the new API. I don't want to do this work now which will muddy the waters of what this PR is about.

Comment on lines +71 to +73
"resolutions": {
"chokidar": "3.4.3"
},
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So, even after all my efforts trying to install next manually, I couldn't get it to work (still got that hunk error) and any yarn from scratch would give that error anyway.

Adding this resolution fixes the issue as suggested here: vercel/next.js#21343 (comment) (although I suspect that typed the version number wrong because they're referring to the new one rather than old one)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For posterity:

As the above linked issue suggests, yarn 2.4.0 fixed that issue but we have another issue which prevents us from updating. parcel-bundler/parcel#4064 (comment)

That seems to have appeared between 2.1.1 (which we are on right now) and 2.2.0.

@@ -1852,7 +1855,7 @@ function scrollAreaStateReducer(
// Alternatively we could reconsider exposing the Position component, or we could allow users to
// pass their own positionRef with a prop.
function useExtendedScrollAreaRef(
forwardedRef: React.ForwardedRef<any>,
forwardedRef: React.Ref<any>,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No idea what happened with this file! But somehow this TS error got flagged because React.ForwardedRef is not a thing. I have no idea how this went through for months…

On top of that all the other changes are just prettier, again not sure how it went in not formatted…

For both reasons above I've declined a package update for this as it's just internal.

@benoitgrelard benoitgrelard merged commit ef8f37d into main Feb 17, 2021
@benoitgrelard benoitgrelard deleted the ssr-setup branch February 17, 2021 17:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants