ProductiveRage / Bridge.React

Bindings for Bridge.NET for React - write React applications in C#!
MIT License
74 stars 14 forks source link

Library breaks when "preserveMemberCase": true in bridge.json #16

Closed Zaid-Ajaj closed 7 years ago

Zaid-Ajaj commented 7 years ago

React.createClass will throw exception complaining about the react component not having a render method. setting preserveMemberCase back to false solves the problem

ProductiveRage commented 7 years ago

I've been trying to reproduce this so that I can consider the options.. but I can't make

"preserveMemberCase": true

work!

I tried adding it to bridge.json in the Bridge.React.Example project but it doesn't seem to change anything. I've created a class in that project -

public sealed class OtherClass
{
    public string GetName() { return "blah"; }
}

And I expected the JavaScript method to be named "GetName" if I set preserveMemberCase (rather than "getName" if I don't set preserveMemberCase).

Before I raise an issue with the Bridge Team about this, are you doing anything different to what I'm doing? It should be a case of just adding "preserveMemberCase": true to the bridge.json file in the project that emits JavaScript, right?

For when I do get this working, what behaviour are you hoping for when setting preserveMemberCase? The React component classes need to have a "render" method because this is what the React library looks for. If you enabled preserveMemberCase on your project, would you be happy for the React methods (render, componentDidUpdate, etc..) continuing to be camel-cased (as React requires) while other methods respecting the preserveMemberCase setting? (I'm not completely sure whether this is possible but I hope it is.. I would expect that any [Name] attributes would take precedence over preserveMemberCase and so I could ensure that I use [Name] attributes on every React method.. but I can't currently test this assumption because I can't get preserveMemberCase to work!).

Zaid-Ajaj commented 7 years ago

@ProductiveRage Apologies for not being thorough about the issue. I am not doing anything different. Just setting the preserveMemberCase to true. I have a typical React app. When preserveMemberCase is true I get the following (I can't share all the code but it look roughly as this):

screenshot

As you can see, life-cycle methods are PascalCased which is why React was unhappy (I think).

The expected behaviour is that the library still works even when preserveMemberCase = true.

[Name]ing the methods sounds like a good idea (I haven't tested it yet).

ProductiveRage commented 7 years ago

The option worked with another project I had, so I've been able to test that [Name] works. Thankfully, it does! If I use [Name] attributes on all React methods in Bridge.React then they will take precedence over the preserveMemberCase option in the project that references Bridge.React.

That's the good news. I think I still need to raise something with the Bridge Team, though, because this preserveMemberCase seems a bit dangerous. For example, if I have something like the following:

MyWebApp references MyLibrary MyWebApp has preserveMemberCase enabled but MyLibrary does not MyLibrary has a static class MyHelper with a method DoWork

Then the "DoWork" method in MyLibrary will be called "doWork" in the JavaScript but MyWebApp will try to call "DoWork" - the changes to case should only be applied to references in the current project, I think, not references in other projects. Just a warning for you, you might encounter the same problem. (See forums.bridge.net/forum/bridge-net-pro/bugs/3314).

I should be able to add the [Name] attributes to Bridge.React and release the update this evening some time (UK time).

ProductiveRage commented 7 years ago

I sat down to look at adding [Name] to the React component classes' public members and then realised that I'd have to also add it to every public member in the entire library! (Unless it had either a [Name] or a [Template] attribute, which the DOM class does, for example).

That would mean every public member of every event class, every property in ReactStyle, every property in every attribute class. That's a lot of public members to change :(

The sensible thing to do would be to automate it. It shouldn't be a huge amount to use Roslyn to parse the files and inject the [Name] attributes where necessary..

For now, I might wait because the Bridge Team said that they're thinking about changing the preserveMemberCase behaviour so that it is "assembly aware" - if you set it for your project and then pull in another library, that library will have its own preserveMemberCase value (either true or false) and then the problem that we're having here won't occur. However, I think that that is only an idea on their part at the moment. I asked in forums.bridge.net/forum/bridge-net-pro/bugs/3314 if they have any sort of timeframe in mind (but that might be asking a bit much if they haven't yet decided what they think is the best way to do it).

Is this a big problem for you currently? I might wait a few days to hear from the Bridge Team before I commit time to trying to insert 100s (1000s?) of [Name] attributes in the Bridge.React library (though that can remain a backup plan in case we need it).

Zaid-Ajaj commented 7 years ago

I see the problem, sounds very tedious to do. I think we could wait for the Bridge team to solve this. Currently it's not a problem for me, my POCO's needed to preserve their PascalCase when shared with the client so I just camelCased them and everything worked without preserveMemberCase.

This preserveMemberCase could prove very annoying for people using this library with little background knowledge about js and react. I suggest for time being to add a note in the README.md to remember not to preserveMemberCase so that users are not totally clueless why their app did not work when they only wanted PascalCased properties :)