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

Upgrade with-redux example to app router #49994

Merged
merged 15 commits into from Jun 16, 2023

Conversation

dvakatsiienko
Copy link
Contributor

What?

Update with-redux example.

Why?

with-redux example have:

  • outdated packages
  • outdated approaches and relies on pages directory

Since app router is stable now and is recommended to use, I've updated this example.

How?

An update includes:

  • move example to app router
  • update package.json deps to the latest versions
  • modernize jest by switching from ts-node to @swc/jest
  • fix and overhaul tests
  • modernize redux infra
  • overhaul example source code quality

@dvakatsiienko dvakatsiienko requested review from a team, leerob and steven-tey as code owners May 18, 2023 13:22
@ijjk ijjk added the examples Issue/PR related to examples label May 18, 2023
@dvakatsiienko dvakatsiienko changed the title Update with-redux example to app router Upgrade with-redux example to app router May 18, 2023
@dvakatsiienko
Copy link
Contributor Author

Hi, @leerob @steven-tey

It seems that Github Actions CI got broken at Socket Security: Pull Request Alerts and it blocks CI pipeline to advance.

This happens for multiple PR's related to examples directory. I've created an issue for that.

@socket-security
Copy link

socket-security bot commented May 26, 2023

New dependency changes detected. Learn more about Socket for GitHub ↗︎


🚨 Potential security issues found in this pull request. To accept the risk, merge this PR and you will not be notified again.

Bot Commands

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of package-name@version specifiers. e.g. @SocketSecurity ignore foo@1.0.0 bar@* or ignore all packages with @SocketSecurity ignore-all

  • @SocketSecurity ignore @swc/core@1.3.60
📜 Install scripts

Install scripts are run when the package is installed. The majority of malware in npm is hidden in install scripts.

Packages should not be running non-essential scripts during install and there are often solutions to problems people solve with install scripts that can be run at publish time instead.

Package Script field Source
@swc/core@1.3.60 (added) postinstall examples/with-redux/package.json via @swc/jest@0.2.26
Pull request alert summary
Issue Status
Install scripts ⚠️ 1 issue
Native code ✅ 0 issues
Bin script shell injection ✅ 0 issues
Unresolved require ✅ 0 issues
Invalid package.json ✅ 0 issues
HTTP dependency ✅ 0 issues
Git dependency ✅ 0 issues
Potential typo squat ✅ 0 issues
Known Malware ✅ 0 issues
Telemetry ✅ 0 issues
Protestware/Troll package ✅ 0 issues

📊 Modified Dependency Overview:

➕ Added Package Capability Access +/- Transitive Count Publisher
@swc/core@1.3.60 filesystem, shell, environment +10 kdy1
@types/redux-logger@3.0.9 None +0 types
waait@1.0.5 None +0 wesbos
react-redux@8.0.5 None +0 acemarke
@swc/jest@0.2.26 filesystem +12 kdy1
jest-watch-typeahead@2.2.2 None +4 simenb

🚮 Removed packages: @testing-library/user-event@13.5.0, ts-jest@27.1.5

@dvakatsiienko dvakatsiienko force-pushed the update-with-redux-example branch 2 times, most recently from b34649c to 8510db0 Compare May 28, 2023 09:00
Copy link
Member

@leerob leerob left a comment

Choose a reason for hiding this comment

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

It would be great to simplify this example. Remote saas, remove testing, and focus specifically on Next.js + Redux only in the most minimal, yet complete, example.

@dvakatsiienko
Copy link
Contributor Author

Sounds like a good idea, I'll simplify it.

@dvakatsiienko
Copy link
Contributor Author

@leerob I've removed tests and sass, see if all good now.

Copy link
Member

@balazsorban44 balazsorban44 left a comment

Choose a reason for hiding this comment

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

Let's drop src as well, to match other examples.

Copy link
Member

@balazsorban44 balazsorban44 left a comment

Choose a reason for hiding this comment

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

Let's not add barrel files (index.ts files that are just re-exports), feels like adding complexity instead of reducing it.

@dvakatsiienko
Copy link
Contributor Author

@balazsorban44 removed src folder, and barrel files.

@ijjk
Copy link
Member

ijjk commented Jun 16, 2023

Allow CI Workflow Run

  • approve CI run for commit: eec7825

Note: this should only be enabled once the PR is ready to go and can only be enabled by a maintainer

@ijjk
Copy link
Member

ijjk commented Jun 16, 2023

Allow CI Workflow Run

  • approve CI run for commit: eec7825

Note: this should only be enabled once the PR is ready to go and can only be enabled by a maintainer

@kodiakhq kodiakhq bot merged commit 04cd1fd into vercel:canary Jun 16, 2023
32 checks passed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
examples Issue/PR related to examples locked
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants