-
-
Notifications
You must be signed in to change notification settings - Fork 83
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: allow users to import from node_modules #1021
feat: allow users to import from node_modules #1021
Conversation
WalkthroughWalkthroughThe updates across various files in the schema package primarily involve enhancing the path resolution and import capabilities, focusing on better integration with Changes
Assessment against linked issues
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
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.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (4)
- packages/schema/src/cli/cli-util.ts (2 hunks)
- packages/schema/src/plugins/prisma/schema-generator.ts (2 hunks)
- packages/schema/src/utils/ast-utils.ts (2 hunks)
- packages/schema/src/utils/pkg-utils.ts (3 hunks)
Additional comments: 8
packages/schema/src/utils/ast-utils.ts (2)
- 21-21: The addition of the
findNodeModulesFile
import frompkg-utils
is necessary for the new functionality introduced in theresolveImportUri
function. This change aligns with the PR's objective to enhance import capabilities, specifically for handling imports fromnode_modules
.- 98-114: The modifications to the
resolveImportUri
function introduce several key enhancements:
- Adding a
.zmodel
extension to paths that don't already end with it.- Handling different path formats (relative, absolute Unix, absolute Windows).
- Utilizing
findNodeModulesFile
for resolving paths withinnode_modules
.These changes are crucial for supporting the feature of importing models from
node_modules
. However, there are a few considerations:
- The check for absolute Windows paths uses a regex that specifically looks for paths starting with a drive letter followed by
:\\
. Ensure this regex accurately captures all valid Windows absolute paths.- The use of
findNodeModulesFile
assumes that if a path does not start with.
,/
, or match the Windows absolute path regex, it must be a module withinnode_modules
. This logic seems sound, but it's important to verify that this assumption holds in all intended use cases.- The function now mutates
imp.path
directly by appending.zmodel
if necessary. While this should work as intended, consider the implications of mutating function arguments and whether this could have unintended side effects elsewhere.Overall, these changes significantly improve the flexibility and capability of the
resolveImportUri
function, aligning with the PR's goals.packages/schema/src/utils/pkg-utils.ts (3)
- 32-40: The refactoring of the
findUp
function introduces support for searching multiple paths and improves clarity and extensibility. This change is significant for the PR's objective to enhance path resolution capabilities. The function now supports returning either a single path or an array of paths, depending on themultiple
flag. This flexibility is crucial for locating files within nested directories, such asnode_modules
. The implementation appears to be correct and efficiently uses recursion to traverse up the directory tree. However, ensure thorough testing, especially for edge cases like symbolic links or permission issues in certain directories.- 56-65: The addition of the
findNodeModulesFile
function is a key part of the PR's objective to enable importing fromnode_modules
. This function uses the enhancedfindUp
to locate thenode_modules
directory and then attempts to find the specified file within it. The logic for splitting the provided name into folder and file parts and constructing the file location is sound. However, consider adding more detailed error handling or logging to help diagnose issues when files cannot be found. Additionally, ensure that this function is thoroughly tested with various directory structures and naming conventions withinnode_modules
.- 136-140: The deprecation notice for
findPackageJson
in favor of usingfindUp
is a good practice, as it encourages the use of the more flexible and generalizedfindUp
function. This change aligns with the PR's goal of improving path resolution logic. Ensure that all references tofindPackageJson
within the project have been updated to usefindUp
, and consider adding a warning log or console output whenfindPackageJson
is used, to alert developers of the deprecation.packages/schema/src/cli/cli-util.ts (1)
- 283-283: The replacement of
findPackageJson
withfindUp
in thegetDefaultSchemaLocation
function is a direct application of the changes made inpkg-utils.ts
. This update leverages the enhanced capabilities offindUp
to locate thepackage.json
file, which is a more flexible and robust approach than the previousfindPackageJson
function. This change is consistent with the PR's objectives and improves the logic for schema location retrieval. Ensure that the rest of the codebase has been updated accordingly to usefindUp
wherefindPackageJson
was previously used.packages/schema/src/plugins/prisma/schema-generator.ts (2)
- 51-51: The import statement for
findUp
from../../utils/pkg-utils
replaces the previousfindPackageJson
. This change aligns with the PR's objective to enhance path resolution logic, particularly for locatingpackage.json
files. Ensure that all instances wherefindPackageJson
was used are now correctly utilizingfindUp
with the appropriate arguments and handling its return values correctly.- 453-453: The usage of
findUp
to locatepackage.json
in the functiongetDefaultPrismaOutputFile
is a direct application of the newly imported utility. This change is crucial for the feature allowing users to import models fromnode_modules
by improving the path resolution logic. It's important to verify that thefindUp
function is correctly implemented to handle both the search forpackage.json
and the resolution of the Prisma schema output file path based on thezenstack.prisma
configuration withinpackage.json
. Additionally, ensure that error handling and edge cases, such as the absence ofpackage.json
or invalid configurations, are adequately addressed.
@ymc9 could you advise where you would like me to write tests in for this, and what specific tests you'd like me to run? Is there a node_module which has a zmodel which we can import as part of the development dependancies or would you like me to initialise a very basic package just to host as that dependancy? |
Currently, the only zmodel that resides in node_modules is "@zenstackhq/openapi/plugin.zmodel". The "plugin.zmodel" will be automatically loaded when a plugin is enabled. I believe you can also use it to test the import mechanism without using the plugin. A bit hacky though. Alternatively, you can also just scaffold an npm project in the test case, contrive a dependency, and smuggle a zmodel inside of it. I think the test cases can belong to "packages/schema/tests/schema/...". What you think? |
1f8bc15
to
83b6f37
Compare
multiple paths and add detailed JSDoc comments for better understanding and documentation
Signed-off-by: Jonathan Stevens <jonathan.stevens@resnovas.com>
…s conditions to handle different scenarios and improve flexibility in importing modules
…ts URI Signed-off-by: Jonathan Stevens <jonathan.stevens@resnovas.com>
83b6f37
to
5a5dcd4
Compare
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.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (2)
- packages/schema/src/utils/ast-utils.ts (2 hunks)
- packages/schema/src/utils/pkg-utils.ts (3 hunks)
Additional comments: 2
packages/schema/src/utils/ast-utils.ts (1)
- 21-21: The addition of
findNodeModulesFile
frompkg-utils
is appropriate for the new functionality introduced inresolveImportUri
. This change aligns with the PR's objective to enhance import capabilities fromnode_modules
.packages/schema/src/utils/pkg-utils.ts (1)
- 135-135: The deprecation of
findPackageJson
in favor offindUp
is noted. This change aligns with the PR's objective to refine type definitions and improve functionality. Ensure that all references tofindPackageJson
are updated to usefindUp
where applicable.Verification successful
Given the findings, it's necessary to further investigate whether the remaining reference to
findPackageJson
is actively used in the codebase or if it's simply its definition. To clarify this, I will search for calls tofindPackageJson
excluding its definition.
The verification process reveals that while
findPackageJson
is still defined in the codebase, there are no active calls to this function outside of its definition. This supports the review comment's implication thatfindPackageJson
has been deprecated in favor offindUp
, aligning with the PR's objective to refine type definitions and improve functionality. Therefore, the review comment is consistent with the findings in the codebase.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to findPackageJson in the codebase. rg --type ts 'findPackageJson'Length of output: 124
Script:
#!/bin/bash # Search for calls to findPackageJson excluding its definition rg --type ts 'findPackageJson' | grep -v 'export function findPackageJson'Length of output: 74
Hi @TGTGamer , thanks for continuing to work on it. I've added a few review comments. |
…ckage-manager-to-provide-exportability
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.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- packages/schema/src/utils/ast-utils.ts (2 hunks)
Additional comments: 2
packages/schema/src/utils/ast-utils.ts (2)
- 21-21: The addition of
findNodeModulesFile
frompkg-utils
is a crucial part of enabling imports fromnode_modules
. Ensure thatpkg-utils
is properly versioned and maintained to avoid potential future compatibility issues.- 98-113: The modifications to
resolveImportUri
introduce more robust handling of import paths, including support fornode_modules
paths. However, there are several points to consider:
- The addition of the
.zmodel
extension (lines 100-101) assumes all imports should end with this extension. Confirm this assumption holds for all use cases.- The logic to determine if a path points to
node_modules
(lines 104-110) is clear. EnsurefindNodeModulesFile
gracefully handles cases where thenode_modules
directory or the specified module does not exist.- The final resolution of the path (line 113) is straightforward. Ensure that error handling is in place for cases where the resolved path does not exist or is inaccessible.
+ try { + const resolvedPath = Utils.resolvePath(dirUri, imp.path); + if (fs.existsSync(resolvedPath)) { + return resolvedPath; + } else { + throw new Error(`Resolved path does not exist: ${resolvedPath}`); + } + } catch (error) { + console.error(`Error resolving import path: ${error}`); + return undefined; + }
- Consider using
path.isAbsolute
for checking absolute paths, as suggested byymc9
in previous comments. This could simplify the checks for absolute paths on different platforms.- && !imp.path.startsWith('/') // Respect absolute paths (Unix) - && !/^[a-zA-Z]:\\/.test(imp.path) // Respect absolute paths (Windows) + && !path.isAbsolute(imp.path) // Respect absolute paths
Signed-off-by: Jonathan Stevens <jonathan.stevens@eventiva.co.uk>
Signed-off-by: Jonathan Stevens <jonathan.stevens@eventiva.co.uk>
Changes made. @ymc9 please review :) |
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.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- packages/schema/src/utils/ast-utils.ts (2 hunks)
- packages/schema/src/utils/pkg-utils.ts (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- packages/schema/src/utils/ast-utils.ts
Additional comments: 4
packages/schema/src/utils/pkg-utils.ts (4)
- 8-9: The
FindUp
type definition is clear and well-documented. It effectively uses TypeScript's conditional types to return either a single string or an array of strings based on the generic parameter. This is a good use of TypeScript's advanced type features to create flexible and reusable utility types.- 17-19: The
findUp
function is well-documented and appears to be a versatile utility for finding files or directories by traversing up the directory tree. The use of generics to allow for returning either a single path or an array of paths is a smart design choice. However, it's important to ensure that this function is thoroughly tested, especially in edge cases such as symbolic links or filesystem permissions issues.- 40-52: The
findNodeModulesFile
function has been updated to userequire.resolve
for resolving module paths, which is a significant improvement over manual path construction. This leverages Node.js's own resolution logic, which is more reliable and handles edge cases better. The change addresses the comment fromymc9
regarding the use ofrequire.resolve
. This is a good example of incorporating feedback into the codebase.- 134-134: The
findPackageJson
function is marked as deprecated in favor of thefindUp
function. This is a good move towards simplifying the codebase and reducing redundancy. However, it's important to ensure that all references tofindPackageJson
throughout the project have been updated to usefindUp
instead. Additionally, consider adding a deprecation warning in the function body to alert developers who might still be using it.
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.
Looks good now! The only remaining thing is the documentation of findNodeModulesFile
is outdated. Please help update and the pr is good to merge!
Thank you!
Signed-off-by: Jonathan Stevens <jonathan.stevens@eventiva.co.uk>
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.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- packages/schema/src/utils/pkg-utils.ts (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- packages/schema/src/utils/pkg-utils.ts
Goal: Closes #1008
Caution
Test for IDE's not completed due to errors running on Windows & Codespace Development Environments.
ERR_PNPM_RECURSIVE_RUN_FIRST_FAIL jetbrains@1.9.0 build: "./gradlew buildPlugin"
Summary
This code defines a function named
resolveImportUri
that takes aModelImport
object as input and returns aURI
object orundefined
. The function is responsible for resolving the import path of theModelImport
object.Example Usage
Code Analysis
Inputs
imp
(type:ModelImport
): TheModelImport
object that contains the import path to be resolved.Flow
path
property of theimp
object is undefined, null, or an empty string. If true, return undefined.path
does not end with '.zmodel', append '.zmodel' to thepath
.path
is a relative path, an absolute path (Unix), or an absolute path (Windows). If true, skip to step 6.findNodeModulesFile
function with thepath
as an argument to find the file location in the node_modules directory.imp
object using thegetDocument
andUtils.dirname
functions.Utils.resolvePath
function with the directory URI and the modifiedpath
.Outputs
URI
object: The resolved URI of the import path.undefined
: If the import path cannot be resolved.Tests
Summary by CodeRabbit
Summary by CodeRabbit
package.json
files across various modules for improved path resolution.findPackageJson
function, encouraging the use of the new path searching methods.