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

[BUGFIX beta] Trim whitespace in Ember.String.w() #14611

Conversation

treznick
Copy link

Currently, Ember.String.w() has a bit of unexpected behavior when used in the context of the needs property of a moduleFor helper. For instance, one might write the following code:

  moduleFor("controller:foo/bar", "foo bar Controller", {
    needs: Ember.String.w(`
             controller:application
             controller:foo
             controller:bar/index
           `)
  });

and expect the result of Ember.String.w() to be ["controller:application", "controller:foo", "controller:bar/index"].

Surprisingly, the result is ["", "controller:application", "controller:foo", "controller:bar/index", ""], which might be fine in and of itself, but blows the call stack of moduleFor :(

This PR suggests that Ember.String.w() trim whitespace before attempting to split on whitespace. It provides the end user a bit more flexibility with whitespacing of the argument string.

I realize that much of the specific pain I encountered could be solved with better whitespacing in the first place, but it does seem a bit odd that Ember.String.w() interprets leading and trailing whitespace as elements of the subsequent list to be respected.

NB: I tagged this as a BUGFIX, but by all rights it could be a feature request. There's nothing broken per se about the behavior as is, but it does seem a bit surprising, especially when compared to other languages that support collection literals (e.g. ruby's percent strings). That said, I'm fairly novice to both the usage and architecture of ember, so if this isn't a behavior that would be desirable, feel free to disagree.

@sukima
Copy link
Contributor

sukima commented Dec 1, 2016

For anyone who notices this PR and is itching to work around it you can do the following:

moduleFor("controller:foo/bar", "foo bar Controller", {
  needs: Ember.String.w(`
           controller:application
           controller:foo
           controller:bar/index
         `.trim())
});

Though I feel that intent behind String.w() is more useful if it auto trims. I can not imagine a need where I want my string to start / end with whitespace and get empty strings as elements. Personally if I ever needed empty strings in my array String.w() would be the last utility I would think of.

@Serabe
Copy link
Member

Serabe commented Jul 24, 2017

Hello @treznick,

in first place let me thank you for this PR. Also, we are really sorry to have let this PR stale and assure you we are working hard to minimise this kinds of situations in the future.

Unfortunately, we are closing this in favor of discussing emberjs/rfcs#236 for deprecating the String extensions and quite likely extracting them to an addon.

Thank you again for this PR!

@Serabe Serabe closed this Jul 24, 2017
@treznick
Copy link
Author

@Serabe no worries. In whatever shakes out of emberjs/rfcs#236 seems a much better place to have these conversations anyway. :)

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.

4 participants