Skip to content

Remove compat javascript behaviour for string concatenation #1682

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

Merged
merged 12 commits into from
Mar 8, 2021
Merged

Remove compat javascript behaviour for string concatenation #1682

merged 12 commits into from
Mar 8, 2021

Conversation

MaxGraey
Copy link
Member

@MaxGraey MaxGraey commented Feb 12, 2021

This pull request removes JS behaviour for null-sh strings like this:

Before (master)

const path = "/dev/" + null; // actually needs changetype<string>(null)
// > "/dev/null"

After (this PR)

const path = "/dev/" + null;
// > "/dev/"

Also removed null checks for concat, startsWith and endsWith.

  • I've read the contributing guidelines

@MaxGraey MaxGraey requested a review from dcodeIO February 12, 2021 22:17
@MaxGraey MaxGraey changed the title Remove leacy javascript behaviour for string concan and startsWith Remove legacy javascript behaviour for string concan and startsWith Feb 12, 2021
@MaxGraey MaxGraey changed the title Remove legacy javascript behaviour for string concan and startsWith Remove legacy javascript behaviour for string concat and startsWith Feb 12, 2021
@MaxGraey MaxGraey changed the title Remove legacy javascript behaviour for string concat and startsWith Remove compat javascript behaviour for string concat and startsWith Feb 12, 2021
@MaxGraey MaxGraey changed the title Remove compat javascript behaviour for string concat and startsWith Remove compat javascript behaviour for string concatenation Feb 13, 2021
@MaxGraey MaxGraey requested a review from dcodeIO February 14, 2021 09:26
@MaxGraey MaxGraey mentioned this pull request Feb 17, 2021
1 task
@MaxGraey
Copy link
Member Author

MaxGraey commented Feb 18, 2021

So anther approach suggested by @vgrichina is throw exception for null strings during concat operation. But in this case we probably should propagate this strict behaviour for rest strings methods as well. Thoughts?

UPD. Pretty many smart people voted for throw an error which more stricter

@saulecabrera
Copy link
Contributor

I had to think about this a bit before voicing my opinion. I believe that in the end, all of this boils down to consistency as @battlelinegames mentioned yesterday in the meeting; if one of the principles of the language is to be as close as possible to JS, then as a user I'd expect these quirks to be there, especially when other typed languages that target JS like TS keep this behavior too. Continuing with the consistency line of thought, if we decide on removing it, then we should clearly state under what principle we decided on removing it, and probably the sanest thing to do moving forward is to apply that "improvement" principle to all the other JS quirks (where possible obviously). In reality, though, improving JS could represent breaking changes, but such discussion falls again under the bucket of what level of compatibility we want to have with it.

TLDR; there's no doubt that this behavior can be improved, and people will vote or share their opinions based on how that improvement can be made, but I think that the bigger discussion here is about the principles of the language rather than how to approach the actual feature.

@MaxGraey
Copy link
Member Author

@dcodeIO could you review this?

@dcodeIO
Copy link
Member

dcodeIO commented Mar 8, 2021

Is it correct to assume, from the diff, that if there is any breaking change in the PR it is in the __gt signature, which is more like a bugfix since __lt doesn't have nullable types?

@MaxGraey
Copy link
Member Author

MaxGraey commented Mar 8, 2021

Yes I guess it's breaking change due to some of signatures was changed

@dcodeIO
Copy link
Member

dcodeIO commented Mar 8, 2021

Considering to label this one a "fix", as it merely fixes that one signature that is different from all the others. Wdyt?

@MaxGraey
Copy link
Member Author

MaxGraey commented Mar 8, 2021

Up to you. I don't think it should significantly affect to existing user's code. So it could be marked as fix or feat (improvments/simplification)

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

Successfully merging this pull request may close these issues.

3 participants