[EDU-1816] - Add NPM Workspaces to examples #2419
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe pull request introduces new Ably environment variables in the main examples configuration and a new React package configuration via a comprehensive Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
@GregHolmes - nice, I like it. Looks pretty clean from a first go of it. To go further though I need VITE_PUBLIC_ABLY_KEY and NEXT_PUBLIC_ABLY_KEY - where can I get those? (sorry, new to examples testing)
As an aside here I would change the dev command for the main docs to be yarn dev (instead of develop) so that it aligns with the conventions here and everywhere else. Tbh though, I have ideas to push for changing the package manager to pnpm across web repos since Yarn 1 is a bit long in the tooth, not sure if you have any views on that. It would be a trivial migration from a workspace perspective (not suggesting we do this now, it's a medium term thing).
I'm also just going to kick off a bot review to see if that sees anything outside of the fairly standard convention I see here
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
examples/pub-sub-connection-state/page.md (1)
88-90: Environment Variable Name Inconsistency
There is an inconsistency in the JavaScript section: step 4 instructs updatingVITE_PUBLIC_API_KEYwhile the JavaScript README usesVITE_PUBLIC_ABLY_KEY. This mismatch may confuse users. Please confirm the intended variable name and update to ensure consistency across documentation.Suggested diff (if
VITE_PUBLIC_ABLY_KEYis intended):-4. In `.env.local` update the value of `VITE_PUBLIC_API_KEY` to be your Ably API key. +4. In `.env.local` update the value of `VITE_PUBLIC_ABLY_KEY` to be your Ably API key.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
examples/pub-sub-connection-state/javascript/yarn.lockis excluded by!**/yarn.lock,!**/*.lockexamples/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (12)
examples/.env.example(1 hunks)examples/package.json(1 hunks)examples/pub-sub-connection-state/javascript/.env.example(0 hunks)examples/pub-sub-connection-state/javascript/README.md(2 hunks)examples/pub-sub-connection-state/javascript/package.json(1 hunks)examples/pub-sub-connection-state/javascript/vite.config.ts(1 hunks)examples/pub-sub-connection-state/page.md(4 hunks)examples/pub-sub-connection-state/react/.env.example(0 hunks)examples/pub-sub-connection-state/react/README.md(2 hunks)examples/pub-sub-connection-state/react/next.config.mjs(1 hunks)examples/pub-sub-connection-state/react/next.config.ts(0 hunks)examples/pub-sub-connection-state/react/package.json(1 hunks)
💤 Files with no reviewable changes (3)
- examples/pub-sub-connection-state/react/.env.example
- examples/pub-sub-connection-state/javascript/.env.example
- examples/pub-sub-connection-state/react/next.config.ts
✅ Files skipped from review due to trivial changes (4)
- examples/.env.example
- examples/pub-sub-connection-state/react/package.json
- examples/pub-sub-connection-state/javascript/package.json
- examples/package.json
🔇 Additional comments (11)
examples/pub-sub-connection-state/react/README.md (2)
16-17: Directory Navigation Update Looks Good
The change to usecd /examples/simplifies navigation and aligns with the new workspace structure.
36-37: Updated Server Startup Command is Clear
The updated commandyarn workspace pub-sub-connection-state-react devclearly reflects the new workspace setup for the React example.examples/pub-sub-connection-state/javascript/README.md (2)
16-17: Consistent Directory Command
Changing tocd /examples/streamlines the instructions and matches the React example.
36-37: Updated Workspace Command for JavaScript
The change toyarn workspace pub-sub-connection-state-javascript devis properly aligned with the new workspace structure.examples/pub-sub-connection-state/page.md (4)
44-45: Unified Navigation for React & JavaScript
The updated commandcd /examples/in the React section simplifies directory navigation and is consistent with the individual README files.
64-65: Updated React Server Command in Page Documentation
Usingyarn workspace pub-sub-connection-state-react devclearly indicates the correct workspace command for the React example.
80-81: Consistent JavaScript Directory Instruction
The change tocd /examples/in the JavaScript section maintains consistency with other documentation.
100-101: Updated JavaScript Workspace Command is Correct
The new commandyarn workspace pub-sub-connection-state-javascript devaccurately reflects the workspace structure for the JavaScript example.examples/pub-sub-connection-state/react/next.config.mjs (2)
1-4: LGTM! Environment variable loading is properly implemented.The implementation correctly loads environment variables from the shared
.env.localfile in the examples root directory, which aligns with the NPM workspace structure.
6-11: Verify the environment variable configuration.The configuration looks good with React strict mode enabled. However, ensure that the Ably key is properly set in the shared
.env.localfile.❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Description: Verify the environment variable configuration # Check if .env.example exists and contains the required variable echo "Checking .env.example for NEXT_PUBLIC_ABLY_KEY..." if ! rg "NEXT_PUBLIC_ABLY_KEY" "examples/.env.example"; then echo "Warning: NEXT_PUBLIC_ABLY_KEY not found in examples/.env.example" fiLength of output: 178
Environment variable configuration is declared correctly in the code, but please double-check your shared
.env.localfile.
- The
next.config.mjsfile properly sourcesNEXT_PUBLIC_ABLY_KEYfromprocess.env.- The execution of the script shows that
examples/.env.examplecontainsNEXT_PUBLIC_ABLY_KEY=, which is typical for a template file.- Verify that the actual Ably key (a non‑empty value) is set in your shared
.env.localfile to ensure it’s available at runtime.examples/pub-sub-connection-state/javascript/vite.config.ts (1)
1-5: LGTM! Environment variable loading is properly implemented.The implementation correctly loads environment variables from the shared
.env.localfile in the examples root directory, which aligns with the NPM workspace structure.
GregHolmes
force-pushed
the
EDU-1816-Create-an-NPM-workspace-usage-for-one-example
branch
from
f96bb29 to
600c0ec
Compare
|
Hey @GregHolmes just checking in on this - this PR looks okay to me and I approve of workspaces here in general, but there's a matter of how we want to proceed with this, given that you say this is a trial in the description. Is your intention to merge this in and then do the other examples in separate PRs in turn, or start another PR that isn't a trial? |
I will update the description as you're happy with this and roll out the changes throughout in this PR just to simplify the release. |
| "pub-sub-connection-state/react", | ||
| "pub-sub-connection-state/javascript" | ||
| ], | ||
| "scripts": { |
There was a problem hiding this comment.
The README materials for the language examples reference yarn workspace pub-sub-connection-state-javascript dev directly and I can't see these scripts used anywhere - so we should either remove these extra scripts, or update the READMEs (unless they're used in another place I'm not aware of)
There was a problem hiding this comment.
I'm not too sure what you mean by this.
These scripts: pub-sub-connection-state-javascript and pub-sub-connection-state-react are referenced in the /examples/pub-sub-connection-state/react and /examples/pub-sub-connection-state/javascript READMEs. As well as in the page.md file in /examples/pub-sub-connection-state/ directory.
These scripts are run in the main /examples/ directory.
There was a problem hiding this comment.
I have left the chat-room-history/ ones in the workspaces. But they'll be added as soon as you're happy with this PR showing one example.
GregHolmes
force-pushed
the
EDU-1816-Create-an-NPM-workspace-usage-for-one-example
branch
3 times, most recently
from
0d4c490 to
0b8810e
Compare
GregHolmes
force-pushed
the
EDU-1816-Create-an-NPM-workspace-usage-for-one-example
branch
4 times, most recently
from
543b2be to
ce59333
Compare
| 5. In a new tab, change directory: | ||
|
|
||
| ```sh | ||
| cd /examples/auth-generate-jwt/server/ | ||
| cd /examples/ |
There was a problem hiding this comment.
You're already in this directory with the workspace changes. I think the auth ones will need rewording as you just need to update the .env files rather than messing around changing directories now.
There was a problem hiding this comment.
Yeah, but if you run yarn run auth-generate-jwt-javascript you're then opening a second terminal to run the server command, which means you'll likely have to navigate to that directory again.
| ``` | ||
|
|
||
| 5. In a new tab, change directory: | ||
|
|
||
| ```sh | ||
| cd /examples/auth-request-token/server/ | ||
| cd /examples/ |
GregHolmes
force-pushed
the
EDU-1816-Create-an-NPM-workspace-usage-for-one-example
branch
from
4f4277d to
97ad0c1
Compare
GregHolmes
deleted the
EDU-1816-Create-an-NPM-workspace-usage-for-one-example
branch
This is a trial to see if this implementation of NPM Workspaces would work.
I have carried this out for pub-sub-connection-state. To test it. You would:
/examples/runyarn installmv /examples/.env.example /examples/.env.local.env.local/examples/runyarn workspace pub-sub-connection-state-react devfor react or runyarn workspace pub-sub-connection-state-javascript devfor javascript.Summary by CodeRabbit
New Features
Chores
Documentation