Conversation
|
Thanks, looks good! Just one question above. |
| 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?
There was a problem hiding this comment.
resolvePath returns an absolute path. This was mainly to output a relative path when --showConfig is passed.
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.
There was a problem hiding this comment.
Oh, wait, for entries node resolution isn't used anyway, so the relative here seems redundant.
There was a problem hiding this comment.
It can be used to get the asconfigDir, which returns an absolute path.
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.
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.
There was a problem hiding this comment.
Ok, let's remove the path.relative here then.
|
Hmm, merging #1411 into here looks like a mistake? |
|
I had forgotten to add the |
|
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. |
|
I think I'll just close this and reopen it to master. |
2 participants
No description provided.