Skip to content
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

✨ Add option to allow relative imports #3

Merged
merged 7 commits into from
Nov 24, 2021
Merged

Conversation

reckter
Copy link
Member

@reckter reckter commented Nov 23, 2021

📦 Published PR as canary version: 0.3.0--canary.3.1498669480.0

✨ Test out this PR locally via:

npm install @opencreek/eslint-plugin-ts@0.3.0--canary.3.1498669480.0
# or 
yarn add @opencreek/eslint-plugin-ts@0.3.0--canary.3.1498669480.0

@reckter reckter added the minor Increment the minor version when merged label Nov 23, 2021
@reckter reckter requested a review from LionC November 23, 2021 12:22
lib/rules/no-relative-imports.ts Outdated Show resolved Hide resolved
lib/rules/no-relative-imports.ts Outdated Show resolved Hide resolved
lib/rules/no-relative-imports.ts Show resolved Hide resolved
lib/rules/no-relative-imports.ts Show resolved Hide resolved
lib/rules/no-relative-imports.ts Outdated Show resolved Hide resolved
lib/rules/no-relative-imports.ts Outdated Show resolved Hide resolved
lib/rules/no-relative-imports.ts Outdated Show resolved Hide resolved
lib/rules/no-relative-imports.ts Outdated Show resolved Hide resolved
lib/rules/no-relative-imports.ts Outdated Show resolved Hide resolved
},
})

return
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should generally try to remove all methods that act on strings when dealing with paths here and try to find safer versions - dealing with paths is very tricky and using string methods always has corner cases

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are only 3 places left where I use "replace".
All of which are removing prefixes from a path.
There is path.relative, but that has applications outside of simply removing a prefix, which I do not want to allow to happen, so I think sticking with replace in these cases instead is the right call.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here - if what we are doing is building a relative path, I think we should do that. What corner cases do you see exactly?

@reckter reckter requested a review from LionC November 23, 2021 17:19
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
lib/index.ts Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@reckter reckter merged commit d3ba8cc into main Nov 24, 2021
@reckter reckter deleted the no-relative-imports branch November 24, 2021 09:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor Increment the minor version when merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants