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

make component names case insensitive. ie: getComponent("fooBaR") #3393

Open
ponelat opened this issue Jul 15, 2017 · 10 comments
Open

make component names case insensitive. ie: getComponent("fooBaR") #3393

ponelat opened this issue Jul 15, 2017 · 10 comments

Comments

@ponelat
Copy link
Member

ponelat commented Jul 15, 2017

The plugin system exposes a way to get components via system.getComponent().
Up till now its been case sensitive. Unfortunately we never put in a style-guide on what case to use.
So we have lowercase, TitleCase components mixed throughout.

I propose we make them case-insensitive.

  • When building the system, we store the downcased name of the component in our object.
  • When fetching, we downcase the argument to getComponent in order to fetch it from our object.

eg:

//...
plugins = [ { components: { wONderBAR: () => <h1> Hi </h1> } } ]
// stored as  { wonderbar: .... }

system.getComponent("wonderBAR") // the component keyed with "wonderbar"

This is to allow any case, and not break any existing code. That said, we should likely normalize the component names in this codebase to be consistent.

Guestimate of work effort: < Normal

@shockey your thoughts?

@shockey
Copy link
Contributor

shockey commented Jul 15, 2017

I think this is a very good idea 😄

@ponelat ponelat changed the title make component names case insensitive. ie: getComponenet("fooBaR") make component names case insensitive. ie: getComponent("fooBaR") Jul 15, 2017
@ghost
Copy link

ghost commented Jul 17, 2017

Hi there! I’m a first-time contributor and was hoping to help out with this issue. I noticed nobody was assigned to it, but if there’s already a solution in progress I’m happy to un-assign myself and try helping out elsewhere. Thanks!

@shockey
Copy link
Contributor

shockey commented Jul 17, 2017

@devthehuman, thanks for checking in! Nobody's working on this, afaik.

Feel free to open a pull request that solves this! Comment here if you need any clarification.

@ghost
Copy link

ghost commented Jul 18, 2017

Thanks! Just a quick question about this first bullet point:

"When building the system, we store the downcased name of the component in our object."

Could you point me in the right direction for where in the build process this should be happening? Is the object you're referring to this base file?

@ponelat
Copy link
Member Author

ponelat commented Jul 18, 2017

hey @devthehuman ... the file ( and function, I think ) you're after lies here... https://github.com/swagger-api/swagger-ui/blob/master/src/core/system.js#L285

That function wraps deepExtend, which is how we combine plugins into one largish object.
The code to be added there would look something like...

 if(isObject(src.components)) { // Bringing in new components?
  dest.components = {...dest.components, downcaseThePropsOf(src.components) } // downcaseTheProps doesn't exist btw. 
  delete src.components // so that we don't add it _again_ later down in this function 
}

There is likely a prettier way, but that's the safest area to add the code, as it's run when we add stuff to the system ( ie: the big object ).

Hope that helps. Stick some console.logs in there to see the stuff that goes through it :)

@shockey
Copy link
Contributor

shockey commented Jul 18, 2017

@devthehuman, not quite - that file holds the data going into the system 😄

"System" is our name for a structure we use to compile all our plugins and presets into one object that will be used to drive the React app. One part of the object is a components object. For each plugin we process, we extend the system object we're building up here: https://github.com/swagger-api/swagger-ui/blob/master/src/core/system.js#L66.

Also, for the getting side of things - you'll want to look at the getComponent definition: https://github.com/swagger-api/swagger-ui/blob/master/src/core/plugins/view/root-injects.js#L99

@shockey
Copy link
Contributor

shockey commented Dec 11, 2017

I'd like to increase the scope a bit here, due to the issue encountered in #3968..

We should be down casing any usage of component names in wrapComponents as well (which has been added since we last discussed this issue) - so that silly case mismatches don't affect that interface, either.

@bestmike007
Copy link
Contributor

@shockey I believe there should be a better way; not everyone prefer case-insensitiveness.

What about introducing a case check in dev environment and verbose the case miss-match on build. These additional code could be excluded from the production build by setting NODE_ENV and adding if blocks, which will be removed by uglifyjs as dead code; similar way as react remove propsType check.

@SampsonCrowley
Copy link

The fix for this should be to name components what their actual name is in my opinion. not have this mix of internal names people need to dig through to know that there's a different name for. or actually have a full list of every component where it is easy to find otherwise

the Operation class should always work for "Operation". the OperationSummary class should always be referred to as "OperationSummary"

it is only the mix of following a convention of getting a component by class name and then some components not being referred to by class name that causes confusion for most developers. I would venture to say that everyone that isn't a dev on this code base would expect getComponent to fetch a component by the name of the class/function

you are getting or overriding a react component, it has to follow specific naming conventions to work with JSX and devs are expecting that those conventional names are what you are referring to items with

@cxx5208
Copy link

cxx5208 commented Feb 15, 2024

Hey Can you assign this to me? I will try to fix the issue
Thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants