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

Remove deprecated compile #137

Conversation

SignpostMarv
Copy link
Contributor

While not strictly necessary, this builds upon #136 in order to verify the refactoring has not affected functionality.

While we don't have #31 closed yet, I'm going to assume that the refactored toRegExp & test methods have the same or lower performance impact as calling RegExp.compile() on VerEx().add().

It will however break anything that expects VerEx() to be an instanceof RegExp.

…ion of unchanged code when adding members to prototype instead of assigning prototype
…ion of unchanged code when adding members to prototype instead of assigning prototype
@softwarespot
Copy link
Contributor

Was there a reason you changed setting the prototype all at one to individually?

VerbalExpression.prototype = {
    anything: function anything() ...
}

// to

VerbalExpression.prototype.anything = function anything() ...

@SignpostMarv
Copy link
Contributor Author

We're using airbnb/legacy for linting, the airbnb style guide on constructors suggests assigning to the prototype object instead of defining it.

@softwarespot
Copy link
Contributor

Ah fair point, kind of missed that part in the style guide as I write in ES2015 these days and not ES5. Just a shame eslint doesn't warn about this.

@jehna
Copy link
Contributor

jehna commented Feb 13, 2016

So this makes the VerbalExpressions behave more like a proper Builder.

I can see this is technically a better approach, since the current implementation requires a heavy injecting method to run each time we create a new object.

However, I'd see we could easily keep this library backwards-compatible by just implementing the native RegExp object's methods so they could be called straight from the VebalExpression object (as you've now done with .test() method). What do you think?

In addition, the README.md file should be changed not to state that this is a native RegExp method.

@SignpostMarv
Copy link
Contributor Author

we've got toString, toSource seems to be firefox-only, compile is deprecated (thus removed), so I guess we just need .exec() ?

If we're taking the backwards compatibility approach of making the RegExp methods available, should we have a plan for required level of support for methods introduced/removed in future standards?

@mihai-vlc
Copy link
Contributor

Is this still relevant or can we close it ?

@mihai-vlc mihai-vlc closed this Aug 16, 2018
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