Skip to content

Fix typo and remove unnecessary eslint comments#4055

Merged
Tyriar merged 4 commits intoxtermjs:masterfrom
GreenMashimaro:fix-typo
Aug 24, 2022
Merged

Fix typo and remove unnecessary eslint comments#4055
Tyriar merged 4 commits intoxtermjs:masterfrom
GreenMashimaro:fix-typo

Conversation

@GreenMashimaro
Copy link
Copy Markdown
Contributor

Description

fix typo and remove unnecessary eslint comments

What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Comment on lines -6 to -10
// eslint-disable-next-line prefer-const
export let promptLabel = 'Terminal input';

// eslint-disable-next-line prefer-const
export let tooMuchOutput = 'Too much output to announce, navigate to rows manually to read';
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This actually needs to be let, it's exposed to embedders of xterm.js so they can change the strings to localize xterm.js. It's exposed (as non-readonly) here

public static get strings(): ILocalizableStrings {
return Strings;
}

Copy link
Copy Markdown
Contributor Author

@GreenMashimaro GreenMashimaro Aug 24, 2022

Choose a reason for hiding this comment

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

Thanks for your reply.

I verified it myself within the project. Validation does not affect the original functional logic.

Verify 1: From the compilation results, the two are the same.

original code

export let promptLabel = 'Terminal input';
export let tooMuchOutput = 'Too much output to announce, navigate to rows manually to read';

after compilation code

"use strict";
Object.defineProperty(exports, "__esModule", { value: true });
exports.tooMuchOutput = exports.promptLabel = void 0;
exports.promptLabel = 'Terminal input';
exports.tooMuchOutput = 'Too much output to announce, navigate to rows manually to read';
//# sourceMappingURL=LocalizableStrings.js.map

original code

export const promptLabel = 'Terminal input';
export const tooMuchOutput = 'Too much output to announce, navigate to rows manually to read';

after compilation code

"use strict";
Object.defineProperty(exports, "__esModule", { value: true });
exports.tooMuchOutput = exports.promptLabel = void 0;
exports.promptLabel = 'Terminal input';
exports.tooMuchOutput = 'Too much output to announce, navigate to rows manually to read';
//# sourceMappingURL=LocalizableStrings.js.map

Verify 2: Use let to declare variables; the value cannot be modified in the import place. The two are the same.

image

Verify 3: The exported const variable can also be modified normally

export const promptLabel = 'Terminal input';
export const tooMuchOutput = 'Too much output to announce, navigate to rows manually to read';

and then, no error prompt

Terminal.strings.promptLabel = 'modify';

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ah I see, still using const shows the wrong intent here. const means this value should not change, and while currently TypeScript outputs is as mutable, it may not in the future. So I think we should keep this as it was

Copy link
Copy Markdown
Contributor Author

@GreenMashimaro GreenMashimaro Aug 24, 2022

Choose a reason for hiding this comment

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

I think your idea is good, and I suggest adding a comment. I re-updated the submission.

@Tyriar Tyriar added this to the 5.0.0 milestone Aug 23, 2022
Copy link
Copy Markdown
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

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

Thanks!

@Tyriar Tyriar merged commit b68f820 into xtermjs:master Aug 24, 2022
@GreenMashimaro GreenMashimaro deleted the fix-typo branch August 24, 2022 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants