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

Fix an issue where module resolution didn't follow symlinks. #4295

Merged
merged 3 commits into from Nov 19, 2018

Conversation

Projects
None yet
5 participants
@iclanton
Copy link
Contributor

iclanton commented Nov 15, 2018

PR checklist

  • Addresses an existing issue: #4294
  • bugfix
    • Includes tests - The tests for this issue ended up being too complicated to be practical
  • Documentation update

Overview of change:

There is an issue where TSLint doesn't correctly resolve packages in a node_modules folder that are symlinked to another location where their dependencies are satisfied. This is how the pnpm arranges its node_modules folder, so this issue is easily reproducible with a tslint.json file that extends a file from another package, which also extends a file from another package.

CHANGELOG.md entry:

[bugfix] Files an issue where TSLint doesn't correctly resolve packages in a node_modules folder that are symlinked to another location where their dependencies are satisfied.

@palantirtech

This comment has been minimized.

Copy link
Member

palantirtech commented Nov 15, 2018

Thanks for your interest in palantir/tslint, @iclanton! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request.

@iclanton

This comment has been minimized.

Copy link
Contributor

iclanton commented Nov 15, 2018

@JoshuaKGoldberg
Copy link
Collaborator

JoshuaKGoldberg left a comment

Changes look good; thanks for sending this in! Just a few stylistic comments (2/3 are nitpicks); if you don't have time to address them soon, I can and then merge this in.

For posterity's sake it's good to note: normally it'd be good to add unit tests, but symlinks on Git can be pretty tricky to set up cross-OS, so it seems fine to leave them out for now.

Show resolved Hide resolved src/utils.ts Outdated
Show resolved Hide resolved src/utils.ts Outdated
Show resolved Hide resolved src/utils.ts Outdated
@iclanton

This comment has been minimized.

Copy link
Contributor

iclanton commented Nov 16, 2018

I tried to put together some unit tests for this, but the test ended up being really complicated and flaky. The test had to do a lot of nontrivial setup work that I wasn't able to get to work deterministically.

@JoshuaKGoldberg
Copy link
Collaborator

JoshuaKGoldberg left a comment

Super, thanks! 🚀

@iclanton

This comment has been minimized.

Copy link
Contributor

iclanton commented Nov 19, 2018

CLA is signed. Once this is merged in, how long does it usually take for TSLint to be released?

@JoshuaKGoldberg

This comment has been minimized.

Copy link
Collaborator

JoshuaKGoldberg commented Nov 19, 2018

Whoohoo, thanks @iclanton!

Normally, TSLint gets released every few months. It's a little late on a 5.12 because there isn't anybody within @palantirtech working on it full time (though they're trying to find someone now).

In the meantime, you can always fork TSLint and publish your own tslint-spdev-fork module with explicit "we're only using this until a new version of TSLint is released!" documentation. Prior art. Not a great solution, so if anybody has other ideas that'd be great.

@JoshuaKGoldberg JoshuaKGoldberg merged commit 1775e4a into palantir:master Nov 19, 2018

12 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
ci/circleci: test2.1 Your tests passed on CircleCI!
Details
ci/circleci: test2.4 Your tests passed on CircleCI!
Details
ci/circleci: test2.7 Your tests passed on CircleCI!
Details
ci/circleci: test2.8 Your tests passed on CircleCI!
Details
ci/circleci: test2.9 Your tests passed on CircleCI!
Details
ci/circleci: test3.0 Your tests passed on CircleCI!
Details
ci/circleci: testNext Your tests passed on CircleCI!
Details
ci/circleci: testRc Your tests passed on CircleCI!
Details
cla/palantir CLA signed on 2018-11-19 20:32 UTC+00:00
Details
@iclanton

This comment has been minimized.

Copy link
Contributor

iclanton commented Nov 19, 2018

Can a patch be released? We'd really rather not create a fork because we have to support TSLint for external customers.

@iclanton iclanton deleted the iclanton:ianc/fix-module-resolve branch Nov 19, 2018

@pgonzal

This comment has been minimized.

Copy link

pgonzal commented Nov 20, 2018

In the meantime, you can always fork TSLint and publish your own tslint-spdev-fork module with explicit "we're only using this until a new version of TSLint is released!" documentation. Prior art. Not a great solution, so if anybody has other ideas that'd be great.

This would require a nontrivial approval process, since it would be released to third parties with support contracts.

@iclanton

This comment has been minimized.

Copy link
Contributor

iclanton commented Nov 28, 2018

Any news on getting this fix published?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment