| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
|
Thanks, looks good! Just one question above. |
Sorry, something went wrong.
| if (asconfig.entries) { | ||
| for (let entry of asconfig.entries) { | ||
| argv.push(optionsUtil.resolvePath(entry, asconfigDir)); | ||
| argv.push(path.relative(baseDir, optionsUtil.resolvePath(entry, asconfigDir))); |
There was a problem hiding this comment.
Why is this necessary when asconfigDir is already relative to baseDir?
Sorry, something went wrong.
There was a problem hiding this comment.
resolvePath returns an absolute path. This was mainly to output a relative path when --showConfig is passed.
Sorry, something went wrong.
There was a problem hiding this comment.
It returns an absolute path only if entry is already absolute, where I guess keeping it that way makes sense for debugging purposes (so one knows about where the entry comes from by looking at the asconfigs), or node resolution is used, which is debatable. In the latter case, the place to change it would be inside of resolvePath where require.resolve is called, though.
Sorry, something went wrong.
There was a problem hiding this comment.
Oh, wait, for entries node resolution isn't used anyway, so the relative here seems redundant.
Sorry, something went wrong.
There was a problem hiding this comment.
It can be used to get the asconfigDir, which returns an absolute path.
Sorry, something went wrong.
There was a problem hiding this comment.
Thanks for clarifying. I'm not necessarily opposed to changing node resolution to relative, but if we do so I think we should do it in resolvePath where require.resolve is used (and the path returned as absolute), so we don't break explicitly specified absolute paths given as the value of extends. Like, if a user specifies something absolute explicitly, I'd prefer to not mangle it but keep it as-is.
Sorry, something went wrong.
There was a problem hiding this comment.
Changed it in #1406 accordingly.
Sorry, something went wrong.
There was a problem hiding this comment.
very true.
Sorry, something went wrong.
There was a problem hiding this comment.
Just looked at your changes to #1406, and I think you should roll it back and just keep everything as is. The issue was that I wanted the path relative to baseDir in asc.js, but in resolvePath, baseDir is passed in as asconfigDir. So we could adapt that to always pass in top level baseDir or just forget about it all together.
Sorry, something went wrong.
There was a problem hiding this comment.
Ok, let's remove the path.relative here then.
Sorry, something went wrong.
|
Hmm, merging #1411 into here looks like a mistake? |
Sorry, something went wrong.
|
I had forgotten to add the node_modules in the test for node resolution. |
Sorry, something went wrong.
|
As noted above, #1411, which is unrelated, somehow made it into this PR. With the original base branch now merged, I suggest to rebase this PR cleanly on master. |
Sorry, something went wrong.
|
I think I'll just close this and reopen it to master. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
No description provided.