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

build: replace 'string' package dependency with custom function to check is a file ends with '.html' or not #1008

Merged
merged 1 commit into from
Jul 29, 2021

Conversation

aleks-elkin
Copy link
Contributor

Because the package grunt-spiritual-edbml-tmpfix hijacking String.prototype with non-standard polyfills it is impossible to use a standard String.prototype.endsWith() function, so I had to implement the custom one-line function to check is the filename ends with .html or not.


What's wrong with the custom implementation in grunt-spiritual-edbml-tmpfix? It looks like this:

String.prototype.endsWith = function (string) {
  return this.indexOf(string) === this.length - 1;
};

The problems are:

  • Anything longer than 1 character will fail the check.
  • If we have two occurrences of the search term in the string it will also fail.

You can see the implementation of String.prototype.endsWith in grunt-spiritual-edbml-tmpfix here: https://unpkg.com/[email protected]/tasks/things/compiler.js

@aleks-elkin aleks-elkin requested a review from a team as a code owner July 29, 2021 12:22
Copy link
Contributor

@wejendorp wejendorp left a comment

Choose a reason for hiding this comment

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

... wow. But nice to get rid of string 👍

@aleks-elkin aleks-elkin merged commit 86fba3f into master Jul 29, 2021
@aleks-elkin aleks-elkin deleted the elkin/remove-string-dep branch July 29, 2021 12:41
@aleks-elkin aleks-elkin mentioned this pull request Nov 9, 2021
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