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

feat: Add NextJS example #367

Merged
merged 7 commits into from
Apr 3, 2023
Merged

feat: Add NextJS example #367

merged 7 commits into from
Apr 3, 2023

Conversation

kbieganowski
Copy link
Member

Summary

Added NextJS example based on Vite tests in examples/web-vite (#226)

nextjs_example

@changeset-bot
Copy link

changeset-bot bot commented Feb 26, 2023

⚠️ No Changeset found

Latest commit: eb88c15

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@kbieganowski kbieganowski changed the title chore: Add NextJS example feat: Add NextJS example Feb 26, 2023
@thymikee
Copy link
Member

thymikee commented Mar 7, 2023

Lovely!

@Xiltyn
Copy link
Collaborator

Xiltyn commented Mar 9, 2023

I believe we still need some things to be complete here @thymikee. I'm working with @kbieganowski on this one ;)

@Xiltyn Xiltyn self-requested a review March 9, 2023 11:36
@Xiltyn
Copy link
Collaborator

Xiltyn commented Mar 9, 2023

Unfortunately, at the moment we're experiencing issues with building the Next example using Bob. Based on the information provided by @kbieganowski, the issues appears to lie in Next custom tags used for things like styling. This is related to how Bob handles babel and the fact, that Next.js currently doesn't play well with this setup.

That being said, because this will always only be a problem in our repo, I recommended to adjust the example so that it doesn't use those custom tags. When a Next.js user downloads reassure package, they will not have this issue, because they will not have to build our internal Next.js example, they would have gotten a built library.

We should at the same time mention this in our documentation so that w don't focus potential users who want to play around. We could also provide a hint on hacking the setup in order to make it work if we know how to do it.

cc: @thymikee @mdjastrzebski

@@ -0,0 +1,38 @@
This is a [Next.js](https://nextjs.org/) project bootstrapped with [`create-next-app`](https://github.com/vercel/next.js/tree/canary/packages/create-next-app).
Copy link
Member

Choose a reason for hiding this comment

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

Pls update this README file to explain that this is plain NextJS app straight from the CLI tool with Reassure atteched. And how to run the reassure tests.

@@ -0,0 +1,33 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

Pls use yarn to install packages, as all other examples use it.

Copy link
Member

Choose a reason for hiding this comment

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

It seems that both yarn.lock and package-lock.json (NPM) are present. Pls remove NPM's one.

Copy link
Member

Choose a reason for hiding this comment

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

Removed

package.json Outdated
@@ -71,7 +71,8 @@
"workspaces": {
"packages": [
"packages/*",
"examples/native"
"examples/native",
"examples/web-nextjs"
Copy link
Member

Choose a reason for hiding this comment

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

Do not add nextjs here. All examples except from examples/native are not using monorepo, as this causes version conflicts for react versions/etc

Suggested change
"examples/web-nextjs"

Copy link
Member

Choose a reason for hiding this comment

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

Removed

package.json Outdated Show resolved Hide resolved
@Xiltyn
Copy link
Collaborator

Xiltyn commented Mar 9, 2023

@kbieganowski
Please update the ESLint setup so that we don't collide with react-native specific rules, e.g. no strings within tags.

cc: @mdjastrzebski

Copy link
Collaborator

@Xiltyn Xiltyn left a comment

Choose a reason for hiding this comment

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

Great job! There are still some small bits to iron out. It looks like the issues with custom tags have been dealt with as far as I can see, nice!

We still need to update ESLint rules and align code styling to match our prettier config.

@mdjastrzebski
Copy link
Member

Looks good! Awesome work @kbieganowski and @Xiltyn.

Let's fix the lint, I think we can simply disable linting in examples/web-nextjs.

Also let's update examples Readme file, to describe our example rather than NextJS default

Copy link
Collaborator

@Xiltyn Xiltyn left a comment

Choose a reason for hiding this comment

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

I believe we're good here, there is a small ReadMe file update, but it's a minor thing I will ammend before merge if necessary.

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

4 participants