Use resolve instead of Node.JS require for resolving configuration files
#1172
Conversation
`resolve` can handle relative and absolute links.
… the main tslint binary
| "Review the Node lookup algorithm (https://nodejs.org/api/modules.html#modules_all_together) " + | ||
| "for the approximate method TSLint uses to find the referenced configuration file."); | ||
| } | ||
| const basedir = relativeTo || process.cwd(); |
There was a problem hiding this comment.
Love how you simplified this code!
I'm wondering if we should also attempt to load with regular require.resolve as well. I.e. does it also make sense to look for the node module relative to the installed location of TSLint? I think it does, but only if the "path" specified is not a path but instead a package reference
There was a problem hiding this comment.
That would mean it's not the same as the [require.resolve() algorithm](https://nodejs.org/dist/latest-v4.x/docs/api/modules.html#modules_all_together). Before this pull request Y(in the algorithm) was always the location of the tslint package, now it's the location of the parenttslint.json` file (or the tslint package the first time).
It's similar to how you'd expect node -e "require('foo')" to fail if foo was installed globally but not locally.
However, it's arguably a breaking change, so I'm happy to add a fallback which resolves with require.resolve to avoid issues for other users.
There was a problem hiding this comment.
Agree with you that ideally extends references in a tslint.json file should only reference packages installed where the file is, but I can see this being a situation where people are relying on other cases, so let's prevent the breaking change here
Clever, seems like a good way to test things out! Seems like it's causing the Windows CI to fail though, any ideas?
|
|
It may be as simple as adding Unfortunately, I can't reproduce the test failure locally, so I'm just guessing at the moment |
Nice fix on this issue, looks like |
And install it as a dev dependency
…he tslint require path.
|
These commits should maintain backwards compatibility with packages installed relative to tslint, with some extra unit tests and CLI tests. |
|
|
||
| # check that a tslint file (outside of the resolution path of the tslint package) can resolve a package that is | ||
| # installed as a dependency of tslint. See palantir/tslint#1172 for details. | ||
| tmpDir=$(mktemp -d) |
There was a problem hiding this comment.
On the version of mktemp installed on my Mac, this is unfortunately an invalid usage. I'm okay with getting rid of this test here though, as you have a similar test below. Plus your test below should work on Windows also, which is great.
|
This looks great to me! I'll make the one change I mentioned above in a follow-up commit |
…files (palantir#1172) * Add `resolve` dependency (and custom typings) * Use resolve instead of require in resolveConfigurationPaths Fixes palantir#1171 * Simplify resolveConfigurationPath `resolve` can handle relative and absolute links. * path-is-absolute is no longer needed. * Install test packages in subdirectory, so they aren't in the scope of the main tslint binary * Run `npm install` in test config directory to install test packages. * Fix code style issue * Add dev dependency on npm for grunt run:installTestDeps task * Revert "Add dev dependency on npm for grunt run:installTestDeps task" This reverts commit fa0b947. * Replace run:installTestDeps task with npm-command:test * Fix lint errors in Gruntfile * Resolve config file relative to the cwd if it can't be found relative to the parent config * Make rules in tslint-test-custom-rules package valid rules (copied from noFailRule) * Add CLI test with a relative extend config * Add test config package And install it as a dev dependency * Add CLI test for extending a package which is installed in tslint. * Make test packages private * Fix entry point of test packages * Simplify non-relative test package * Add unit test for loadConfigurationFromPath with configs outside of the tslint require path.
Fixes #1171 by using resolve to resolve package-based extension configuration files from the directory of the current configuration file.
I've modified the tests for extends so that they have their own node_modules (created by the
run:installTestDepstask) in./test/config, so thattslint-test-configandtslint-test-custom-rulescan't be accessed by tslint directly withrequire().