fix: npm link issue #244
fix: npm link issue #244
Conversation
WalkthroughThe pull request introduces several modifications across multiple files, primarily focusing on updating import paths for consistency and clarity. Key changes include the addition of instructions in the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (5)
packages/invoice-dashboard/vite.wc.config.ts (1)
5-7: Consider documenting npm link usageSince this configuration enables npm link functionality, it would be helpful to document the setup process for other developers.
Consider adding a section in the README or CONTRIBUTING guide about:
- How to use npm link with this project
- Why preserveSymlinks is necessary
- Any gotchas or considerations when using linked packages
packages/payment-widget/vite.wc.config.ts (1)
Line range hint
13-16: Consider documenting polyfill requirementsThe package requires specific node polyfills (buffer, crypto) while explicitly excluding others. This architectural decision should be documented.
Consider adding technical documentation explaining:
- Why these specific polyfills are needed
- Security implications of using crypto polyfills in browser
- Why certain polyfills are explicitly excluded
README.md (2)
35-37: Consider adding troubleshooting sectionSince npm linking can be tricky, consider adding a troubleshooting section that covers common issues and their solutions, such as:
- Module resolution errors
- Version mismatch problems
- Peer dependency conflicts
Line range hint
58-58: Fix typo in documentationThere's a typo in the word "pacakges" - it should be "packages".
-pacakges/<component>/README.md +packages/<component>/README.mdpackages/shared/icons/network/mantle.svelte (1)
14-28: Enhance SVG accessibility and optimize markup.Consider the following improvements:
- Add descriptive
aria-labelattributes to paths for better accessibility- Simplify styling by applying fill directly to paths
- Add meaningful IDs to paths for better maintainability
- <style type="text/css"> - .st0 { - fill: #000; - } - </style> - <path - class="st0" + <path + id="mantle-outer-ring" + aria-label="Mantle logo outer ring" + fill="#000" d="M155.5,289.1l0-45.7c-4.6,0-9.3-0.4-13.9-1.1l-7.2,45.1C141.4,288.6,148.5,289.1,155.5,289.1L155.5,289.1z M176.5,287.5c7-1.1,13.9-2.7,20.6-4.9L183,239.1c-4.4,1.4-9,2.5-13.6,3.2L176.5,287.5z M113.9,282.6l14.1-43.4 c-4.4-1.4-8.7-3.2-12.9-5.3l-20.8,40.7C100.6,277.7,107.2,280.4,113.9,282.6z M216.6,274.6c6.3-3.2,12.3-6.9,18-11l-27-36.9 c-3.7,2.7-7.7,5.2-11.9,7.2L216.6,274.6z M76.3,263.5l27-36.9c-3.7-2.7-7.3-5.7-10.6-9l-32.4,32.3l0,0 C65.3,254.8,70.7,259.4,76.3,263.5z M250.5,250.1l0.3-0.3c5-4.9,9.6-10.3,13.7-15.9l-37-26.9c-2.7,3.7-5.8,7.3-9,10.5l-0.5,0.5 l15.6,14.7L250.5,250.1z M46.6,233.8l37-26.9c-2.7-3.7-5.2-7.7-7.3-11.8l-40.8,20.7C38.7,222.1,42.4,228.1,46.6,233.8L46.6,233.8z M275.5,215.9c3.2-6.2,5.9-12.8,8.1-19.5L240,182.4c-1.4,4.4-3.2,8.7-5.3,12.8L275.5,215.9z M27.4,196.4L71,182.3 c-1.4-4.4-2.5-8.9-3.2-13.5l-45.3,7.1C23.6,182.8,25.3,189.7,27.4,196.4L27.4,196.4z M288.5,176c1.1-6.9,1.7-14,1.7-21l-45.8,0 c0,4.6-0.4,9.3-1.1,13.9L288.5,176z M66.7,154.9c0-4.6,0.4-9.3,1.1-13.9l-45.3-7.1c-1.1,6.9-1.6,14-1.7,21L66.7,154.9L66.7,154.9z M243.3,141.1l45.3-7.1c-1.1-6.9-2.7-13.8-4.9-20.5l-43.6,14C241.5,132,242.6,136.5,243.3,141.1L243.3,141.1z M71,127.5 c1.4-4.4,3.2-8.7,5.3-12.8L35.5,94c-3.2,6.2-5.9,12.8-8.1,19.5L71,127.5L71,127.5z M234.8,114.8l40.9-20.7 c-3.2-6.3-6.9-12.3-11.1-18L227.5,103C230.2,106.7,232.7,110.7,234.8,114.8L234.8,114.8z M83.6,102.9c2.7-3.7,5.8-7.3,9.1-10.6 l0.1-0.1L60.4,59.9L60.3,60c-5,5-9.6,10.3-13.7,16L83.6,102.9z M218.5,92.5l9.8-9.9l22.5-22.4l-0.1-0.1c-5-4.9-10.3-9.5-15.9-13.6 l-27,36.9c3.7,2.7,7.3,5.7,10.5,9L218.5,92.5z M103.4,83.2c3.7-2.7,7.7-5.1,11.9-7.2L94.5,35.3c-6.3,3.2-12.3,6.9-18,11L103.4,83.2z M196,76.1l20.9-40.7c-6.3-3.2-12.8-5.9-19.5-8.1l-14.2,43.4C187.5,72.2,191.8,74,196,76.1L196,76.1z M128.2,70.7 c4.4-1.4,9-2.5,13.6-3.2l-7.1-45.1c-6.9,1.1-13.9,2.7-20.6,4.9L128.2,70.7z M169.5,67.5l7.2-45.1c-6.9-1.1-14-1.7-21-1.7l0,45.7 C160.3,66.4,165,66.8,169.5,67.5z" /> - <path - class="st0" + <path + id="mantle-inner-ring" + aria-label="Mantle logo inner ring" + fill="#000" d="M155.5,245.5v-45.9c-3.9,0-7.7-0.5-11.5-1.5l-12,44.3C139.7,244.4,147.6,245.5,155.5,245.5z M179,242.4 c7.7-2,15-5,21.9-9l-23.2-39.7c-3.3,1.9-6.9,3.4-10.6,4.4L179,242.4z M110.2,233.4l23.2-39.7c-3.3-1.9-6.4-4.3-9.2-7l-0.4-0.4 l-14.6,15.3l-18,17.3l0.3,0.3C97.1,224.7,103.4,229.5,110.2,233.4L110.2,233.4z M219.6,219.2L219.6,219.2 c5.6-5.6,10.5-11.8,14.4-18.6l-40.1-23c-1.9,3.3-4.3,6.3-7.1,9.1L219.6,219.2z M77,200.5l40.1-22.9c-1.9-3.3-3.4-6.8-4.4-10.5 L68,178.9C70,186.4,73.1,193.7,77,200.5z M243.1,179c2-7.6,3.1-15.4,3.1-23.2l-46.3,0c0,3.8-0.5,7.7-1.5,11.4L243.1,179z M111.3,155.7c0-3.8,0.5-7.7,1.5-11.4L68,132.5c-2,7.6-3.1,15.4-3.1,23.2L111.3,155.7L111.3,155.7z M198.3,144.4l44.8-11.8 c-2-7.6-5.1-14.9-9-21.7l-40.1,22.9C195.8,137.2,197.3,140.7,198.3,144.4z M117.2,133.8c1.9-3.2,4.2-6.3,7-9l0.1-0.1l-16.6-16.4 L91.8,92l-0.3,0.3c-5.6,5.5-10.4,11.8-14.4,18.5L117.2,133.8z M107.7,108.3l0.2,0.2L107.7,108.3z M186.8,124.8l16.4-16.3L220,92.7 l-0.4-0.4C214,86.7,207.7,82,200.9,78l-23.2,39.7c3.3,1.9,6.3,4.2,9.1,6.9L186.8,124.8z M133.4,117.7c3.3-1.9,6.9-3.4,10.6-4.4 L132.2,69c-7.7,2-15,5-21.9,9L133.4,117.7z M167,113.4l12-44.3c-7.6-2-15.5-3.1-23.4-3.1l0,45.9C159.4,111.9,163.3,112.4,167,113.4 L167,113.4z" />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (4)
package-lock.jsonis excluded by!**/package-lock.jsonpackages/shared/assets/logo-sm.svgis excluded by!**/*.svgpackages/shared/assets/logo.svgis excluded by!**/*.svgpackages/shared/fonts/Montserrat-VariableFont_wght.ttfis excluded by!**/*.ttf
📒 Files selected for processing (25)
README.md(1 hunks)package.json(3 hunks)packages/create-invoice-form/src/lib/create-invoice-form.svelte(1 hunks)packages/create-invoice-form/src/lib/invoice/form-view.svelte(1 hunks)packages/create-invoice-form/src/lib/invoice/form.svelte(1 hunks)packages/create-invoice-form/src/lib/utils/prepareRequest.ts(1 hunks)packages/create-invoice-form/vite.wc.config.ts(1 hunks)packages/invoice-dashboard/package.json(1 hunks)packages/invoice-dashboard/src/lib/dashboard/drawer.svelte(1 hunks)packages/invoice-dashboard/src/lib/dashboard/invoice-view.svelte(1 hunks)packages/invoice-dashboard/src/lib/view-requests.svelte(1 hunks)packages/invoice-dashboard/src/utils/formatAddress.ts(1 hunks)packages/invoice-dashboard/vite.wc.config.ts(1 hunks)packages/payment-widget/src/lib/components/payment-complete.svelte(1 hunks)packages/payment-widget/src/lib/components/payment-confirmation.svelte(1 hunks)packages/payment-widget/src/lib/components/wallet-info.svelte(1 hunks)packages/payment-widget/src/lib/payment-widget.svelte(1 hunks)packages/payment-widget/src/lib/utils/currencies.ts(1 hunks)packages/payment-widget/vite.wc.config.ts(1 hunks)packages/shared/icons/network/mantle.svelte(1 hunks)packages/shared/icons/network/moonbeam.svelte(1 hunks)packages/shared/package.json(1 hunks)shared/icons/package.json(0 hunks)shared/types/package.json(0 hunks)shared/utils/package.json(0 hunks)
💤 Files with no reviewable changes (3)
- shared/icons/package.json
- shared/utils/package.json
- shared/types/package.json
✅ Files skipped from review due to trivial changes (15)
- packages/invoice-dashboard/src/utils/formatAddress.ts
- packages/shared/package.json
- packages/shared/icons/network/moonbeam.svelte
- packages/payment-widget/src/lib/utils/currencies.ts
- packages/payment-widget/src/lib/components/payment-confirmation.svelte
- packages/payment-widget/src/lib/components/wallet-info.svelte
- packages/payment-widget/src/lib/components/payment-complete.svelte
- packages/create-invoice-form/src/lib/invoice/form.svelte
- packages/create-invoice-form/src/lib/create-invoice-form.svelte
- packages/invoice-dashboard/src/lib/dashboard/invoice-view.svelte
- packages/create-invoice-form/src/lib/invoice/form-view.svelte
- packages/invoice-dashboard/src/lib/dashboard/drawer.svelte
- packages/payment-widget/src/lib/payment-widget.svelte
- packages/create-invoice-form/src/lib/utils/prepareRequest.ts
- packages/invoice-dashboard/src/lib/view-requests.svelte
🔇 Additional comments (6)
packages/invoice-dashboard/vite.wc.config.ts (1)
5-7: LGTM! Verify symlink resolution behavior
The addition of preserveSymlinks: true is the correct approach to fix npm link issues. This allows Vite to maintain the original module paths when resolving symlinked packages.
Let's verify the npm link configuration:
✅ Verification successful
LGTM! Symlink configuration is consistent and documented
The preserveSymlinks: true configuration is properly implemented:
- It's consistently used across all web component packages (payment-widget, invoice-dashboard, create-invoice-form)
- The repository has proper workspace configuration (
"workspaces": ["./packages/*"]) - README.md includes clear documentation for npm link usage with specific instructions for web components
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if package.json contains proper workspace configuration
rg -l "workspaces" package.json
# Look for npm link related documentation
rg -l "npm link|yarn link" README.md CONTRIBUTING.md docs/
Length of output: 161
Script:
#!/bin/bash
# Check workspace configuration in package.json
cat package.json | jq '.workspaces'
# Check README.md content for npm/yarn link documentation
rg -A 5 -B 5 "npm link|yarn link" README.md
# Check for any symlink related configurations in other Vite configs
fd -e ts -e js "vite.*config" | xargs rg "preserveSymlinks"
Length of output: 1154
packages/create-invoice-form/vite.wc.config.ts (1)
5-7: LGTM! Consistent configuration
The implementation matches the other Vite configs, maintaining consistency across packages.
packages/payment-widget/vite.wc.config.ts (1)
6-8: LGTM! Configuration aligns with other packages
The implementation is consistent with other Vite configs while maintaining the necessary node polyfills for this package.
package.json (1)
8-8: LGTM: Workspace configuration aligns with package restructuring
The removal of "./shared/" from workspaces and consolidation under "./packages/" supports better npm linking by ensuring a more standard package structure.
README.md (1)
35-37: LGTM: Clear instructions for local package development
The added instructions for using local packages are clear and comprehensive, making it easier for developers to work with local dependencies.
packages/shared/icons/network/mantle.svelte (1)
1-28: SVG implementation looks good overall!
The SVG markup is well-structured and correctly implements the Mantle network logo. The suggested improvements above are optional optimizations that don't affect the core functionality.
| "turbo": "^2.0.6", | ||
| "graphql": "16.8.1" |
There was a problem hiding this comment.
💡 Codebase verification
Remove the graphql dependency as it's not directly used in the codebase
The graphql package appears to be a transitive dependency required by other packages (like graphql-request and graphql-tag) rather than being directly used in the codebase. Since these packages manage their own GraphQL dependency requirements through their peerDependencies, there's no need to explicitly declare graphql in the root package.json.
🔗 Analysis chain
Verify the necessity and version of the graphql dependency
The addition of graphql dependency seems unrelated to the npm linking fix. Please clarify:
- Why this dependency is needed
- If version 16.8.1 is the latest stable version
Let me check if there are actual GraphQL usage patterns in the code, not just references in package files.
Let me fix the syntax error and try again.
Let me try one final search without file type restrictions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if graphql is used in the codebase
rg -l "graphql"
# Check latest version and security advisories
curl -s https://registry.npmjs.org/graphql/latest | jq '.version'
Length of output: 121
Script:
#!/bin/bash
# Search for GraphQL schema definitions, queries, or imports
rg -l "from ['\"]graphql['\"]|require\(['\"]graphql['\"]\)|import.*graphql|gql`|schema.*graphql"
# Check if there are .graphql files
fd -e graphql
# Look for GraphQL type definitions or operations
ast-grep --pattern 'gql`$$$`'
ast-grep --pattern 'import { $$ } from "graphql"'
Length of output: 116
Script:
#!/bin/bash
# Search for GraphQL schema definitions, queries, or imports
rg -l 'from "graphql"|require\("graphql"\)|import.*graphql'
# Check if there are .graphql files
fd -e graphql
# Look for GraphQL related code
rg -A 3 'graphql' --type ts --type js --type tsx --type jsx
Length of output: 165
Script:
#!/bin/bash
# Search for any GraphQL usage in the codebase
rg -A 3 'graphql' --type-not lock --type-not json
# Check package-lock.json for GraphQL dependencies
rg '"graphql"' package-lock.json -A 5
Length of output: 1149
| @@ -40,6 +40,7 @@ | |||
| "@requestnetwork/payment-detection": "0.47.0", | |||
| "@requestnetwork/payment-processor": "0.50.0", | |||
| "@requestnetwork/request-client.js": "0.52.0", | |||
| "@requestnetwork/shared": "*", | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using a more specific version constraint
Using "*" for @requestnetwork/shared version could lead to unexpected behavior:
- It may pull in incompatible versions during npm install
- It could complicate npm linking if versions mismatch
Consider using a specific version range (e.g., "^1.0.0") to ensure stability.
There was a problem hiding this comment.
Does this actually work? I remember @aimensahnoun tried this when he was first setting up the shared packages
There was a problem hiding this comment.
@MantisClone, I see. If using * for @requestnetwork/shared is required to maintain the npm link between the SDK and the web components, and previous attempts with specific versions didn't work, then it's acceptable to keep it as *.
Thanks for letting me know.
(_/)
(•_•)
(/ )
✏️ Learnings added
Learnt from: MantisClone
PR: RequestNetwork/web-components#244
File: packages/invoice-dashboard/package.json:43-43
Timestamp: 2024-12-07T00:39:09.756Z
Learning: When using npm link between the SDK and web components, specifying `*` as the version constraint for `@requestnetwork/shared` is necessary, as using specific versions can cause issues.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| fill: #000; | ||
| } | ||
| </style> | ||
| <rect width="0" height="311" /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unnecessary empty rectangle element.
The rectangle element with width="0" serves no visual purpose and should be removed to reduce markup.
- <rect width="0" height="311" />
There was a problem hiding this comment.
This looks fine to me - as long as it works after linking it to Request Invoicing. 🤔 @rodrigopavezi make sure you test before merging.
Context: @aimensahnoun had issues getting the shared packages to work properly in the Request Invoicing.
|
Please also make sure it doesn't release a shared package to NPM. We wanted to avoid a separate shared package from being created. |
|
Looks good to me (assuming it works) please test to make sure 🙏 |
Problem
Npm link between SDK and Web components were broken
Change
Allow symlink on vite build by adding the following configuration
That required changing the share code into a proper package.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Chores
preserveSymlinksoption in Vite configuration for both the invoice dashboard and payment widget.@requestnetwork/shared-icons,@requestnetwork/shared-types, and@requestnetwork/shared-utils.