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

RSG puts components into global namespace #325

Closed
n1313 opened this issue Feb 14, 2017 · 11 comments
Closed

RSG puts components into global namespace #325

n1313 opened this issue Feb 14, 2017 · 11 comments

Comments

@n1313
Copy link
Collaborator

n1313 commented Feb 14, 2017

All components that are loaded into the styleguide are put into the global namespace, so I think it is possible to accidentally overwrite properties of window with an unfortunately named component.

@sapegin
Copy link
Member

sapegin commented Feb 14, 2017

A good solution would be to extend code that we use for the context option. But it’d be tricky ;-)

@n1313
Copy link
Collaborator Author

n1313 commented Feb 15, 2017

For historical purposes here's my initial stab at this issue. It works but is a bit too dirty of a hack.

@rcline
Copy link
Contributor

rcline commented Oct 17, 2017

@n1313 That really doesn't look that hacky to me. Is there any side effects I am not seeing?

I would love to see this get added! (Currently having to rename my Image component so that it doesn't conflict with window.Image 😞 )

@sapegin
Copy link
Member

sapegin commented Oct 18, 2017

@rcline Doesn’t look super hacky to me now, and I’m sure we can avoid a global variable here. Feel free to send a pull request for that ;-)

@rcline
Copy link
Contributor

rcline commented Oct 18, 2017

@sapegin It looks like this code has been changed/moved.

with (stateWrapper) {
return eval(${JSON.stringify(compiledCode)})
}

Any direction on where to look to make a similar change? (passing in namespaced components to a with () context call)

@sapegin
Copy link
Member

sapegin commented Oct 19, 2017

Not sure which code has been moved where ;-)

I think the right place to do it is here:

// Append React to context modules, since it’s required for JSX
const fullContext = Object.assign({ React: 'react' }, config.context);

But it’ll require some experiments, you’ll need to move props-loader call there I think:

props: requireIt(`!!${propsLoader}!${filepath}`),

@kopax
Copy link
Collaborator

kopax commented Feb 28, 2018

I am also having that issues, I'm not sure if I understand the fix, did anybody tried @sapegin solution?

@linde12
Copy link

linde12 commented Mar 6, 2018

Yup. This makes things very difficult when your library has dependencies to third party code. E.g. in my specific case i have a dependency to a component which in turn depends on a component named Object resulting in React saying "Object.defineProperty is not a function"

@ZxBing0066

This comment has been minimized.

@mirague

This comment has been minimized.

@styleguidist styleguidist locked as off-topic and limited conversation to collaborators Apr 20, 2018
@sapegin
Copy link
Member

sapegin commented Apr 20, 2018

Feel free to send a pull request with a fix, commenting that you also have a known issue won't help anyone.

@sapegin sapegin added this to the 8.0.0: New editor milestone Aug 7, 2018
@sapegin sapegin closed this as completed May 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

7 participants