Skip to content
This repository has been archived by the owner on Mar 25, 2021. It is now read-only.

[quotemark] Excuse more backtick edge cases #4642

Merged
merged 5 commits into from Apr 16, 2019
Merged

[quotemark] Excuse more backtick edge cases #4642

merged 5 commits into from Apr 16, 2019

Conversation

ericbf
Copy link
Contributor

@ericbf ericbf commented Apr 5, 2019

This edge cases were previously flagged when they should be ignored, as changing them breaks typescript. This commit makes it so they are ignored. It also organizes a little better, using functions instead of multi-layered conditionals (it was getting confusing).

PR checklist

Overview of change:

I keep finding other edge cases that I previously missed. I think this is all of them now, but we shall see. Related to #4588.

Is there anything you'd like reviewers to focus on?

See if y'all find any other edge cases.

CHANGELOG.md entry:

[bugfix] Excuse more quotemark backtick edge cases and fix behavior for TS < 2.7.1

Copy link
Contributor

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

A little confused about the 2.7.1 change. Some discussion (preferably in an issue) would be good.

src/rules/noInferredEmptyObjectTypeRule.ts Outdated Show resolved Hide resolved

return (
// In typescript below 2.7.1, don't change values either
ts.version < "2.7.1" ||
Copy link
Contributor

Choose a reason for hiding this comment

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

Directly checking ts.version is a little unusual... maybe there's a cleaner way? Thoughts @adidahiya?

Also, @ericbf, could you elaborate on why this is necessary?

Copy link
Contributor Author

@ericbf ericbf Apr 7, 2019

Choose a reason for hiding this comment

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

Typescript below the listed version fails compilation (or otherwise misbehaves) when backticks are used in some places where it succeeds in future versions. For instance, typeof str === `string`​ does not narrow the type of str in TS versions below 2.7.1, causing compilation errors if you try to use it as a string later. In the other cases, older TS throws errors where NoSubstitutionLiterals are used instead of single/double quoted strings, but works fine in above 2.7.1; rather than cripple the rule to support order versions, I split it up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See the typeof issue here. Its milestone was 2.7 (hence the split in this PR).
And here's the issue for backticks strings as types. This one was 2.6, but rather than doing multiple splits, I just did one at 2.7.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created corresponding issue, #4645

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks! This LGTM but before merging I'd like to hear from adidahiya on whether there's a cleaner way to check on these things.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree it's unusual and should be avoided if possible, but this seems like a reasonable use case for checking ts.version.

However, can you please use the semver package and the helper function from parse.ts to check version ranges instead?

import { lt } from "semver";
import { getNormalizedTypescriptVersion } from "../../verify/parse";

function hasOldTscBacktickBehavior() {
    return lt(getNormalizedTypescriptVersion(), "2.7.1");
}

@ericbf
Copy link
Contributor Author

ericbf commented Apr 10, 2019

Any update here? @adidahiya? @JoshuaKGoldberg?

@adidahiya adidahiya self-assigned this Apr 10, 2019
@ericbf
Copy link
Contributor Author

ericbf commented Apr 15, 2019

Any update here?

This edge cases were previously flagged when they should be ignored, as changing them breaks typescript. This commit makes it so they are ignored. It also organizes a little better, using functions instead of multi-layered conditionals (it was getting confusing).
This commit adds different tests for different version ranges, as those versions behave differently.

return (
// In typescript below 2.7.1, don't change values either
ts.version < "2.7.1" ||
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree it's unusual and should be avoided if possible, but this seems like a reasonable use case for checking ts.version.

However, can you please use the semver package and the helper function from parse.ts to check version ranges instead?

import { lt } from "semver";
import { getNormalizedTypescriptVersion } from "../../verify/parse";

function hasOldTscBacktickBehavior() {
    return lt(getNormalizedTypescriptVersion(), "2.7.1");
}

@adidahiya
Copy link
Contributor

Also if you merge master you'll get the new required CI check, "clean-lockfile"

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[quotemark] backtick breaks old typescript
3 participants