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

Fix typo and remove unnecessary eslint comments #4055

Merged
merged 4 commits into from
Aug 24, 2022

Conversation

GreenMashimaro
Copy link
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
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
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
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
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.

src/common/buffer/BufferSet.ts Show resolved Hide resolved
@Tyriar Tyriar added this to the 5.0.0 milestone Aug 23, 2022
Copy link
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