-
Notifications
You must be signed in to change notification settings - Fork 4
ENG-757 experiment with pnpm #352
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
Changes from all commits
a6b59ba
71448a4
392e244
fff8975
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,29 +9,36 @@ | |
| "build": "tsx scripts/build.ts", | ||
| "lint": "eslint .", | ||
| "lint:fix": "eslint . --fix", | ||
| "publish": "tsx scripts/publish-obsidian.ts", | ||
| "publish": "tsx scripts/publish.ts --version 0.1.0", | ||
| "check-types": "tsc --noEmit --skipLibCheck" | ||
| }, | ||
| "keywords": [], | ||
| "author": "", | ||
| "license": "Apache-2.0", | ||
| "devDependencies": { | ||
| "@octokit/core": "^6.1.2", | ||
| "@repo/eslint-config": "workspace:*", | ||
| "@repo/typescript-config": "workspace:*", | ||
| "@types/node": "^20", | ||
| "@types/react": "catalog:obsidian", | ||
| "@types/react-dom": "catalog:obsidian", | ||
| "autoprefixer": "^10.4.21", | ||
| "builtin-modules": "3.3.0", | ||
| "dotenv": "^16.4.5", | ||
| "esbuild": "0.17.3", | ||
| "eslint": "catalog:", | ||
| "obsidian": "^1.7.2", | ||
| "postcss": "^8.5.3", | ||
| "tailwindcss": "^3.4.17", | ||
| "tslib": "2.5.1", | ||
| "tsx": "^4.19.2", | ||
| "typescript": "5.5.4" | ||
| "typescript": "5.5.4", | ||
| "zod": "^3.24.1" | ||
| }, | ||
| "dependencies": { | ||
| "react": "^19.0.0", | ||
| "react-dom": "^19.0.0", | ||
| "nanoid": "^4.0.2", | ||
| "react": "catalog:obsidian", | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we bump obsidian to 19.1 to consolidate website/obsidian on the same version?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm all for it; but @trangdoan982 should check it if there are side-effects there.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Explicitly: let's leave it at that for now, then try update in another PR. |
||
| "react-dom": "catalog:obsidian", | ||
| "tailwindcss-animate": "^1.0.7" | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,7 +9,7 @@ This directory contains a script for publishing the Discourse Graph Obsidian plu | |
|
|
||
| ## Prerequisites | ||
|
|
||
| 1. **Node.js and npm** - Ensure you have Node.js 18+ installed | ||
| 1. **Node.js and pnpm** - Ensure you have Node.js 20+ installed, with pnpm@10 installed globally (`npm install -g pnpm`) | ||
| 2. **Git** - For repository operations | ||
| 3. **GitHub Token** - For authentication (see Authentication section) | ||
|
|
||
|
|
@@ -42,7 +42,7 @@ The script automatically determines release type based on version format and alw | |
| ### BRAT Version Priority | ||
|
|
||
| BRAT uses alphabetical ordering for pre-release identifiers. This is why we enforce: | ||
| - **Internal**: `alpha-*` (comes first alphabetically) | ||
| - **Internal**: `alpha-*` (comes first alphabetically) | ||
| - **External**: `beta-*` (comes after alpha, gets higher priority) | ||
|
|
||
| This ensures that external beta releases always take precedence over internal alpha releases for BRAT auto-updates. | ||
|
|
@@ -53,7 +53,7 @@ The script automatically determines the release type based on version format: | |
|
|
||
| **🧪 Pre-release** (automatic detection) | ||
| - **Triggers**: Any version with `alpha` or `beta` suffixes | ||
| - **GitHub**: Marked as "Pre-release" | ||
| - **GitHub**: Marked as "Pre-release" | ||
| - **Main Branch**: Not updated (keeps stable code in main) | ||
|
Comment on lines
54
to
57
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Inconsistent “beta” release status between sections Above you state pre-releases (alpha/beta) are marked as “Pre-release”, but the table shows “GitHub Release: ✅ Stable” for beta. Align to “🧪 Pre-release” for beta rows. -| `x.y.z-beta.n` | External Pre-release | ✅ Yes | ❌ No | ✅ Stable | `1.0.0-beta.1` |
+| `x.y.z-beta.n` | External Pre-release | ✅ Yes | ❌ No | 🧪 Pre-release | `1.0.0-beta.1` |Also applies to: 101-106 🧰 Tools🪛 LanguageTool[grammar] ~55-~55: There might be a mistake here. (QB_NEW_EN) [grammar] ~56-~56: There might be a mistake here. (QB_NEW_EN) [grammar] ~57-~57: There might be a mistake here. (QB_NEW_EN) 🤖 Prompt for AI Agents
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. out of PR scope. This was only included because of automated spacing adjustments.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @maparent Understood! You're absolutely right that this documentation inconsistency is out of scope for the PNPM migration PR. Would you like me to create a follow-up issue to track the beta release documentation inconsistency for future cleanup? |
||
| - **Use Case**: Testing, beta versions, internal releases | ||
|
|
||
|
|
@@ -122,7 +122,7 @@ tsx scripts/publish.ts --version 0.1.0-alpha-canvas-feature --release-name "Canv | |
| tsx scripts/publish.ts --version 0.1.0-canvas-feature | ||
| ``` | ||
|
|
||
| ### Beta Release for Public Testing | ||
| ### Beta Release for Public Testing | ||
| ```bash | ||
| # ✅ Correct format with beta prefix - creates pre-release automatically | ||
| tsx scripts/publish.ts --version 1.0.0-beta.1 --release-name "Beta: New Graph View" | ||
|
|
@@ -137,18 +137,18 @@ tsx scripts/publish.ts --version 1.0.0-test.1 | |
| tsx scripts/publish.ts --version 1.0.0 | ||
| ``` | ||
|
|
||
| ### Using npm script from obsidian directory | ||
| ### Using pnpm script from obsidian directory | ||
| ```bash | ||
| cd apps/obsidian | ||
| npm run publish -- --version 1.0.0-beta.1 | ||
| pnpm run publish -- --version 1.0.0-beta.1 | ||
| ``` | ||
|
Comment on lines
+140
to
144
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Verification agent 🧩 Analysis chainPublish script invocation may pass duplicate --version The README suggests If you choose to remove the hardcoded version (preferred), apply this in apps/obsidian/package.json (see separate comment there). 🏁 Script executed: #!/bin/bash
# Inspect the "publish" script in apps/obsidian/package.json to verify if it hardcodes a version flag
grep -R '"publish"' -n apps/obsidian/package.json -A5Length of output: 218 Remove hardcoded We’ve confirmed that but the README instructs users to run: pnpm run publish -- --version 1.0.0-beta.1which results in two To resolve this, please remove the hardcoded • File: - "publish": "tsx scripts/publish.ts --version 0.1.0"
+ "publish": "tsx scripts/publish.ts"After this change, the README snippet remains correct and users can set any version via: cd apps/obsidian
pnpm run publish -- --version 1.0.0-beta.1🤖 Prompt for AI Agents
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Someone more familiar with that part of the codebase should implement a better solution, or mark it as needed for later.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @maparent Understood! This is indeed a technical debt item that can be addressed later. The current hardcoded version won't break functionality, just creates a confusing developer experience. Would you like me to create a GitHub issue to track this improvement? I can document the problem (duplicate 🧠 Learnings used |
||
|
|
||
| ## What the Script Does | ||
|
|
||
| ### For All Releases: | ||
| 1. **Validates input** - Checks version format and arguments | ||
| 2. **Detects release type** - Internal vs External based on version format | ||
| 3. **Builds the plugin** - Runs `npm run build` to create distribution files | ||
| 3. **Builds the plugin** - Runs `pnpm run build` to create distribution files | ||
| 4. **Copies source files** - Copies plugin source (excluding build artifacts) | ||
| 5. **Creates GitHub release** - Always creates a release with: | ||
| - Custom or default release name | ||
|
|
@@ -184,7 +184,7 @@ Publishes to: `DiscourseGraphs/discourse-graph-obsidian` | |
| ### Repository State by Release Type: | ||
|
|
||
| **Internal Release**: Repository unchanged, GitHub pre-release created | ||
| **External Pre-release**: Repository unchanged, GitHub pre-release created | ||
| **External Pre-release**: Repository unchanged, GitHub pre-release created | ||
| **External Stable**: Repository main branch updated + GitHub stable release created | ||
|
|
||
| ## Troubleshooting | ||
|
|
@@ -195,16 +195,16 @@ Publishes to: `DiscourseGraphs/discourse-graph-obsidian` | |
| ```bash | ||
| # ❌ Wrong | ||
| tsx scripts/publish.ts | ||
| # ✅ Correct | ||
|
|
||
| # ✅ Correct | ||
| tsx scripts/publish.ts --version 1.0.0 | ||
| ``` | ||
|
|
||
| 2. **"Invalid version format"** | ||
| ```bash | ||
| # ❌ Wrong | ||
| tsx scripts/publish.ts --version "beta-1" | ||
|
|
||
| # ✅ Correct | ||
| tsx scripts/publish.ts --version 1.0.0-beta.1 | ||
| ``` | ||
|
|
@@ -219,7 +219,7 @@ Publishes to: `DiscourseGraphs/discourse-graph-obsidian` | |
| - Verify repository exists and is accessible | ||
|
|
||
| 5. **"Required build files missing"** | ||
| - Run `npm run build` manually to check for build errors | ||
| - Run `pnpm run build` manually to check for build errors | ||
| - Ensure TypeScript compiles without errors | ||
|
|
||
| 6. **BRAT picking wrong version** | ||
|
|
@@ -247,10 +247,10 @@ This is why the naming convention is critical for ensuring the right version get | |
|
|
||
| ### Key Functions: | ||
| - `isExternalRelease()` - Determines if version is external (stable/beta) or internal (alpha) | ||
| - `updateMainBranch()` - Uses GitHub API to create proper commits | ||
| - `updateMainBranch()` - Uses GitHub API to create proper commits | ||
| - `createGithubRelease()` - Creates releases with assets and automatic pre-release detection | ||
| - `updateManifest()` - Updates version in manifest.json | ||
|
|
||
| ### Security: | ||
| - Uses GitHub API instead of git commands for better security | ||
| - Never commits tokens to repository | ||
| - Never commits tokens to repository | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,5 +12,8 @@ | |
| "importHelpers": true, | ||
| "isolatedModules": true, | ||
| "strictNullChecks": true, | ||
| "paths": { | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For the record: changes to these files were necessary at some point, but not anymore. Could revert. But It think declaring types explicitly is useful. |
||
| "~/*": ["./src/*"] | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,5 @@ | ||
| #!/bin/bash | ||
| set -e | ||
| npm install | ||
| pnpm install | ||
| npx turbo run build --filter=roam | ||
| cp dist/* . | ||
| cp dist/* . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as discussed: the publish script requires a version. This probably had issues before.