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

Trailing slashes in target of dojo.store.JsonStore (Issue #16691) #14

Closed
wants to merge 4 commits into from

Conversation

@josch1710
Copy link
Contributor

josch1710 commented Jul 11, 2013

Fixes issue #16691

Added property allowNoTrailingSlash to control the behaviour. Default is false.
If false, the previous behaviour is used, i.e. a trailing slash on target is expected.
If true, target is checked for a trailing slash. If none exists, then a slash appended to target
If options.before is set and contains an id, then this id is also appended to the target URL.

@josch1710

This comment has been minimized.

Copy link
Contributor Author

josch1710 commented Jul 11, 2013

@ghost ghost assigned kriszyp Jul 16, 2013
@wkeese

This comment has been minimized.

Copy link
Member

wkeese commented Aug 21, 2013

The main issue I have is that there are no tests, so I don't know if the code works or not. JsonRest itself has tests but just against the file system. I guess you would need to write a test PHP server to truly check this change?

@josch1710

This comment has been minimized.

Copy link
Contributor Author

josch1710 commented Aug 22, 2013

1.) I added that, because DnD-Support adds "before" to the options of the store calls. Furthermore, dijit.Tree adds "parent" to the options of the store calls. Both are not handled by the standard JsonRest class.
Any suggestions how to address this issue?
2.) I can't tell you at this moment, why I wrapped it in a string, but I'm sure I needed that.
Why "" + this.target and not new String(this.target)? Is there a difference?
3.) I can change that.
4.) I don't see, where getTarget checks on allowNoTrailingSlash. Can you please specify a line number?
5.) Well, JsonRest tests would need that anyway. I could propably build something in PHP.

@wkeese

This comment has been minimized.

Copy link
Member

wkeese commented Aug 22, 2013

For future reference, it's better if you reply to line notes (https://github.com/josch1710/dojo/commit/59b4bbadefd520a352e7693967c76fcb1ef69e9a#store-jsonrest-js-P18) individually, next to the code the notes are about.

1.) I added that, because DnD-Support adds "before" to the options of the store calls. Furthermore, dijit.Tree adds > "parent" to the options of the store calls. Both are not handled by the standard JsonRest class.
Any suggestions how to address this issue?

I don't know what's up with "before" or "parent" parameters, but I assume they are orthogonal to supporting trailing slashes, so they shouldn't be part of this patch.

Why "" + this.target and not new String(this.target)? Is there a difference?

"" + s is equivalent to new String(s), just a few characters shorter (and therefore better).

4.) I don't see, where getTarget checks on allowNoTrailingSlash. Can you please specify a line number?

Oh, you are right, I misread the code a bit... let me change my question/comment: since getTarget() handles targets both with and without a trailing slash, what's the point of having a allowNoTrailingSlash option at all?

@kfranqueiro

This comment has been minimized.

Copy link
Member

kfranqueiro commented Aug 22, 2013

"before" is a thing specific to dgrid's DnD extension. I have argued previously on the ML that it doesn't necessarily warrant being supported out of the box by dojo/store when it is not that common of a feature. I could see argument for making sure it's easy to extend JsonRest to add support for it (if it isn't already easy to do that), but either way that is completely irrelevant to the issue of allowing a trailing slash on the target.

@csnover

This comment has been minimized.

Copy link
Member

csnover commented Aug 22, 2013

Why "" + this.target and not new String(this.target)? Is there a difference?

"" + s is equivalent to new String(s), just a few characters shorter (and therefore better).

No, it’s not. The former creates a string primitive, the latter creates a String object. One never wants a String object, so new String should never be used.

Also, allowNoTrailingSlash is a really bad property name because it is a positive-negative. According to the Dojo convention (“Negated boolean variable names MUST be avoided: isNotError and isNotFound are unacceptable.”) it should be something like forceTrailingSlash that defaults to true or omitTrailingSlash that defaults to false.

@josch1710

This comment has been minimized.

Copy link
Contributor Author

josch1710 commented Aug 28, 2013

@csnoner
1.) Why would I never use a String object?
2.) I didn't choose the property name. It's an old feature of dojo and referenced in the issue, I wanted to patch.
I'm very open to other property names, if that is better.

@csnover

This comment has been minimized.

Copy link
Member

csnover commented Aug 28, 2013

1.) Why would I never use a String object?

> new String("foo") === "foo"
false
> typeof new String("foo")
object
> console.log(new String("foo"))
{ '0': 'f', '1': 'o', '2': 'o' }

2.) I didn't choose the property name. It's an old feature of dojo […]

Most of the code in dojox is crap which is why it was never promoted to dojo core. Historical context is fine, but violations of the style guide aren’t allowed in core.

- Removed handling of options.
@josch1710

This comment has been minimized.

Copy link
Contributor Author

josch1710 commented Aug 29, 2013

@csnover
1.) I'm not seeming to be the Javascript guru you are, but I don't get how your comment answers my question.
It might be obvious to you, it's not to me. Can you just explain to me, where the problem with String objects is.
I don't get it from your examples. Is it slower? Does it behave in a totally other way than native strings?

@ALL
Would be expectTrailingSlash with a default of true acceptable?

@wkeese

This comment has been minimized.

Copy link
Member

wkeese commented Aug 29, 2013

Would be expectTrailingSlash with a default of true acceptable?

I just don't see any reason for a parameter at all. Why not just make it work correctly if there is or isn't a trailing slash in the href?

@josch1710

This comment has been minimized.

Copy link
Contributor Author

josch1710 commented Aug 29, 2013

@wkeese I really don't care.
I've done it that way because it might be preferable to be able to select the behaviour.
But I think, it should be done in a method, because then target URL creation is interceptable and extendable.

@josch1710

This comment has been minimized.

Copy link
Contributor Author

josch1710 commented Aug 30, 2013

I removed the parameter completely. Now every use of target is replaced by an call to getTarget.

@kfranqueiro

This comment has been minimized.

Copy link
Member

kfranqueiro commented Aug 30, 2013

  • The options argument of getTarget is no longer used. Remove it.
  • getTarget probably shouldn't be marked as a public API. Maybe _getTarget (or _normalizeTarget)?
  • Using match is about the slowest imaginable way to do what you are doing. Why not just get the last character of the string with slice?
  • If the hasId check were kept in its original location in put, there would be no need for the _getTarget method to have to check whether id is not undefined; it would assume it is defined because all uses of it would pass it at all times.
  • Actually, now that I'm thinking about it... would it be preferable to just normalize the target during construction, make sure it has an ending slash like the existing code expects, and be done with it? No need to call a method every time... The only downside would be that target would not exactly match what was provided, but I'm not sure that's a big deal in this case. (Do people ever change the target after construction? We could add a setTarget method instead...)
@josch1710

This comment has been minimized.

Copy link
Contributor Author

josch1710 commented Sep 2, 2013

@kfranqueiro
1.) Then it can't be overriden in a sensible way, e.g. for DnD. That was my starting point.
2.) If it is permissable by Dojo's convention to override such a method, there's no problem to change it.
3.) I will look into it.
4.) Well, if you want to drag some external item into a component, then it probably have no id, but a parent or before object. Doing what you propose, would break that.
5.) As soon as you want to use DnD, that will break. Then we're back to the starting point.

@josch1710

This comment has been minimized.

Copy link
Contributor Author

josch1710 commented Apr 7, 2014

Ping

@dylans

This comment has been minimized.

Copy link
Member

dylans commented Apr 9, 2014

@josch1710 I don't think all of @kfranqueiro's feedback was addressed, was it?

@dylans

This comment has been minimized.

Copy link
Member

dylans commented Apr 9, 2014

Also, please verify if you have a CLA on file ( https://github.com/dojo/dojo/blob/master/CONTRIBUTING.md ), or at least tell us your real name.

@dylans dylans added this to the 1.10 milestone Apr 9, 2014
@dylans

This comment has been minimized.

Copy link
Member

dylans commented May 7, 2014

CLA verified for Jochen Schäfer

// The identity of the requested target
// options: Object
// Options to the target URL generation. Supported is 'before'.
options = options || {};

This comment has been minimized.

Copy link
@wkeese

wkeese May 7, 2014

Member

options isn't used at all (unless I missed something)?

This comment has been minimized.

Copy link
@josch1710

josch1710 May 9, 2014

Author Contributor

At the moment, it isn't used, but:
1.) That can change, or
2.) More important, it's overridable, so I wanted to make the possible use of options explicit.

kriszyp added a commit that referenced this pull request May 8, 2014
@kriszyp

This comment has been minimized.

Copy link
Member

kriszyp commented May 8, 2014

Committed this with cleanup.

@kriszyp kriszyp closed this May 8, 2014
wmfeht added a commit to wmfeht/dojo that referenced this pull request Jul 11, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.