fix(o3r): replaced path.resolve with path.join for better path resolution #1782
Conversation
| @@ -46,10 +46,10 @@ function ngGenerateServiceFn(options: NgGenerateServiceSchematicsSchema): Rule { | |||
| let packageName = destination; | |||
| if (workspaceProject?.projectType !== 'application') { | |||
| let rec = '..'; | |||
| while (!tree.exists(path.resolve(destination, rec, 'package.json')) && path.resolve(destination, rec) !== '/') { | |||
| while (!tree.exists(path.join(destination, rec, 'package.json')) && path.resolve(destination, rec) !== '/') { | |||
There was a problem hiding this comment.
| while (!tree.exists(path.join(destination, rec, 'package.json')) && path.resolve(destination, rec) !== '/') { | |
| while (!tree.exists(path.join(destination, rec, 'package.json')) && path.join(destination, rec) !== '/') { |
There was a problem hiding this comment.
@matthieu-crouzet can't use "path.join" while comparing with root path as we need absolute path. Path.join returns relative path. Hence using path.resolve
There was a problem hiding this comment.
But you are comparing with the tree path that is a relative path to the project
on Windows you will never have a path matching '/'
We don't want to resolve a path outside the project
There was a problem hiding this comment.
Cool.. will take a look prevent it from resolving paths outside projects
gavadhany
force-pushed
the
bugfix/service-path-resolver
branch
from
f97502a
to
771e53a
Compare
gavadhany
force-pushed
the
bugfix/service-path-resolver
branch
from
771e53a
to
a59c26b
Compare
|
@matthieu-crouzet and @kpanot : I have refactored the code a bit. The current version of the code wouldnt work if package.json was in the path entered by user e.g. './'. |
| currentDirectory = path.join(currentDirectory, parent); | ||
| } | ||
| if (currentDirectory === parent) { | ||
| throw new SchematicsException('Could not find package.json in the project directory.'); |
There was a problem hiding this comment.
| throw new SchematicsException('Could not find package.json in the project directory.'); | |
| throw new O3rCliError('Could not find package.json in the project directory.'); |
from @o3r/schematics
| let rec = '..'; | ||
| while (!tree.exists(path.resolve(destination, rec, 'package.json')) && path.resolve(destination, rec) !== '/') { | ||
| rec = path.join('..', rec); | ||
| let currentDirectory = path.join(destination); |
There was a problem hiding this comment.
what is the purpose of path.join here? Maybe you wanted to used path.normalize?
There was a problem hiding this comment.
Yea.. wanted to normalize. Replaced it with normalize
| while (!tree.exists(path.resolve(destination, rec, 'package.json')) && path.resolve(destination, rec) !== '/') { | ||
| rec = path.join('..', rec); | ||
| let currentDirectory = path.join(destination); | ||
| let parent = '..'; |
There was a problem hiding this comment.
the value never change, it should be const.
(I am surprised it has not been caught by the linter :S)
| let currentDirectory = path.join(destination); | ||
| let parent = '..'; | ||
| // Find the package.json file in the project directory | ||
| while (currentDirectory !== parent && !tree.exists(path.join(currentDirectory, 'package.json'))) { |
There was a problem hiding this comment.
did you test that currentDirectory !== parent is correctly executing in the different OS?
It seems to be quite weak condition, maybe a .include(parent) would be a bit stronger?
There was a problem hiding this comment.
I haven't tested on different OSs. But did run with path.posix implementation to verify and compare the results.
I can change it to includes if its stronger
There was a problem hiding this comment.
@kpanot : Changed it from === to includes
gavadhany
force-pushed
the
bugfix/service-path-resolver
branch
3 times, most recently
from
b07a13a
to
5ee911b
Compare
gavadhany
force-pushed
the
bugfix/service-path-resolver
branch
from
5ee911b
to
869eb9f
Compare
kpanot
force-pushed
the
bugfix/service-path-resolver
branch
from
869eb9f
to
3dfaf78
Compare
Proposed change
Change path resolver from path.resolve to path.join as the tree.exists is unable to resolve the absolute path generated by path.resolve
Related issues