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(lib): Fix RegExp constructor with string|RegExp and flags #30586

Merged

Conversation

ExE-Boss
Copy link
Contributor

@ExE-Boss ExE-Boss commented Mar 25, 2019

Previously:

export function replace(str: string, from: string | RegExp, to: string): string {
	return String(str).replace(RegExp(from, 'g'), to);
}

Would result in:

error TS2345: Argument of type 'string | RegExp' is not assignable to parameter of type 'string'.
  Type 'RegExp' is not assignable to type 'string'.

 return String(str).replace(RegExp(from, 'g'), to);
                                   ~~~~

This fixes that.

See also: #14107


review?(@sandersn)

@ExE-Boss ExE-Boss mentioned this pull request Apr 1, 2019
13 tasks
@ExE-Boss
Copy link
Contributor Author

ExE-Boss commented Apr 1, 2019

Obviously a less band‑aid solution would be identifying constructors that can be merged and dynamically merging them for union type calls, but I don’t really know how to do that.

@sandersn
Copy link
Member

sandersn commented Apr 2, 2019

Would you mind explaining the problem more thoroughly. Normally we discuss a change like this with an issue, but this is minor enough it might be OK just to have a thorough explanation of the problem and the fix in the PR description.

@ExE-Boss
Copy link
Contributor Author

ExE-Boss commented Apr 2, 2019

@sandersn

The issue here is that TypeScript doesn’t actually consider the two following cases equal:

interface RegExpConstructor {
	new (pattern: RegExp, flags?: string): RegExp;
	(pattern: RegExp, flags?: string): RegExp;

	new (pattern: string, flags?: string): RegExp;
	(pattern: string, flags?: string): RegExp;
}
interface RegExpConstructor {
	new (pattern: RegExp | string, flags?: string): RegExp;
	(pattern: RegExp | string, flags?: string): RegExp;
}

The second one can be used with:

var pattern: RegExp | string;
var regexp = RegExp(pattern, 'g');

while the first one can’t.


#14107 describes this in more detail.

@sandersn
Copy link
Member

sandersn commented Apr 5, 2019

@DanielRosenwasser do you have opinions? We need to look at the combined overload set and think about what happens if existing code starts resolving to a different overload.

@ExE-Boss
Copy link
Contributor Author

ExE-Boss commented Apr 21, 2019

@sandersn There shouldn’t be any difference, except that my above snippet will work for projects using ES2015 or newer without needing to cast from to any.

Ar do a pointless typeof ternary expression, which is so much worse because it introduces a runtime performance penalty, rather than just a source code ugliness penalty: #14107 (comment).

@RyanCavanaugh
Copy link
Member

@typescript-bot test this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Apr 30, 2019

Heya @RyanCavanaugh, I've started to run the extended test suite on this PR at 6f64fed. You can monitor the build here. It should now contribute to this PR's status checks.

@sandersn
Copy link
Member

@typescript-bot test this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 13, 2019

Heya @sandersn, I've started to run the extended test suite on this PR at 6f64fed. You can monitor the build here. It should now contribute to this PR's status checks.

@sandersn
Copy link
Member

RWC and user test suites are clean. I'm surprised that we don't have coverage of this anywhere in the test suite, but it's clean too.

@sandersn sandersn merged commit 81f7153 into microsoft:master Jun 13, 2019
@ExE-Boss ExE-Boss deleted the lib/es2015/regexp-fix-constructor branch June 13, 2019 22:50
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.

4 participants