Skip to content

Conversation

@ghost
Copy link

@ghost ghost commented Nov 20, 2014

Is this (one of) the causes of issue #1048 ?

Please do NOT pull just yet as there are follow-up bugs which I didn't fix yet. Just wanted feedback whether I'm going in the right direction.

@christopherthielen
Copy link
Contributor

Nice catch. I'm actually working on this code right now.

This is how I modified it:

    this.encode = arrayHandler(bindTo(type, type.encode));
    this.decode = arrayHandler(bindTo(type, type.decode));
    this.is     = arrayHandler(bindTo(type, type.is), true);
    this.equals = arrayEqualsHandler(bindTo(type, type.equals));
    this.pattern = type.pattern;
    this.$arrayMode = mode;

@ghost
Copy link
Author

ghost commented Nov 20, 2014

Hi Christoph, my patch actually is against master not one of the releases, so I think I got your change already. The code you posted above still seems to be affected as it binds the functions rather than keeping the object in the closure. (Sorry, I had to edit the text because I couldn't finish due to a "forced" software update.)

@christopherthielen
Copy link
Contributor

@Jerico-Dev I'm not convinced that's necessary. However, don't spend too much time on it, as I've made significant changes to that area of the code that I haven't yet committed. I plan to commit later tonight.

@christopherthielen
Copy link
Contributor

@Jerico-Dev I looked at your test and now see what you mean. Thank you!

@ghost
Copy link
Author

ghost commented Nov 20, 2014

For now I desisted using custom types as I had difficulties in formatting URLs (the custom type API was called sometimes with encoded values on encode() and sometimes with the decoded values on decode() and both on is(). Guess you're straightening this out in 0.2.13, so I'll hold on. If you like you can pull the fix then...

@christopherthielen
Copy link
Contributor

FYI, .encode() gets called with encoded values. It is sorta like a .normalize() fn. However, .decode() should not be called with pre-decoded value.

@christopherthielen
Copy link
Contributor

merged in via e9f8b8a

@ghost
Copy link
Author

ghost commented Nov 22, 2014

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.

1 participant