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

Getting rid of default exports 💡 #761

Closed
divyanshu-rawat opened this issue Apr 9, 2020 · 17 comments
Closed

Getting rid of default exports 💡 #761

divyanshu-rawat opened this issue Apr 9, 2020 · 17 comments
Assignees
Labels
discussion enhancement New feature or improvement of existing feature. migration Migration from existing stack to new stack. no-issue-activity

Comments

@divyanshu-rawat
Copy link
Member

Is your feature request related to a problem? Please describe.
Currently we are exporting some components as default & some components as const.

Describe the solution you'd like
Now we plant to completely get rid of default export at all, reason being are stated in the blog attached in additional context.

Describe alternatives you've considered
N.A

Additional context
https://blog.neufund.org/why-we-have-banned-default-exports-and-you-should-do-the-same-d51fdc2cf2ad

@divyanshu-rawat divyanshu-rawat added discussion enhancement New feature or improvement of existing feature. migration Migration from existing stack to new stack. labels Apr 9, 2020
@divyanshu-rawat
Copy link
Member Author

I will soon create a PR for some components then anyone who is willing to take up this issue can follow the same process. cc - @Paarmita , @gittysachin , @Marvin9

@divyanshu-rawat divyanshu-rawat self-assigned this Apr 9, 2020
@divyanshu-rawat
Copy link
Member Author

To ensure better dev experience if we are exporting a single component out of directory then we will name the file that is exporting the component as index.tsx instead of naming it as a name of the component.

@Marvin9
Copy link
Member

Marvin9 commented Apr 9, 2020

Agreed.
What should be our approach on components with error boundries? Should be stick with export default there?

@divyanshu-rawat
Copy link
Member Author

example of exporting const with HOC.

I am updating the PR.


import React from 'react';
import {Link} from 'react-router-dom';
import {Container, Paragraph, TopSection, BottomSection} from '../styles';
import {Button} from '../../../ignitus-Shared/ignitus-DesignSystem/ignitus-Atoms/buttons';
import {withErrorBoundary} from '../../../ignitus-Shared/ignitus-ErrorHandlingComponents/errorBoundary';

export const GetStarted: React.FunctionComponent = withErrorBoundary(() => (
  <Container>
    <TopSection>
      <Paragraph>
        Get started for free - join thousands of students and researchers
        already using Ignitus to share their knowledge, work together, and get
        amazing opportunites.
      </Paragraph>
    </TopSection>
    <BottomSection>
      <Link to="/Signup">
        <Button category="orange" size="large">
          Sign-Up
        </Button>
      </Link>
    </BottomSection>
  </Container>
));

@divyanshu-rawat
Copy link
Member Author

We will be exporting everything as const. what do you say @Marvin9 ?

@divyanshu-rawat
Copy link
Member Author

divyanshu-rawat commented Apr 9, 2020

I am also deleting selectors, sagas, actionTypes etc in those file that will never be calling any server endpoint, I will pushing major cleanup in develop soon.

@Marvin9
Copy link
Member

Marvin9 commented Apr 9, 2020

I strongly agree with migration to export const.

@divyanshu-rawat
Copy link
Member Author

Great 👍

@divyanshu-rawat
Copy link
Member Author

divyanshu-rawat commented Apr 9, 2020

export const along with index.tsx/index-ts inside Components directory, instead of creating 2 files one for exporting the other, I will be sending PR soon for some components.

@Marvin9
Copy link
Member

Marvin9 commented Apr 9, 2020

Sure. Do we need migrate slowly? Or in one PR.

@divyanshu-rawat
Copy link
Member Author

I will migrate some then you can pick up some :)

@Marvin9
Copy link
Member

Marvin9 commented Apr 11, 2020

@divyanshu-rawat

By wrapping component in withErrorBoundry, props definitions are being untracked.

export const Login: React.FC<LogInProps> = withErrorBoundary(({ logInRequest, studentLoginData, clearPreviousLogin }) => {

Solution could be

export const Login: React.FC = withErrorBoundary(({ logInRequest, studentLoginData, clearPreviousLogin }: LogInProps) => {

However I think It will become cluttered. What do you think?

@divyanshu-rawat
Copy link
Member Author

@Marvin9 So it is the question of code readability right ? or props definitions are being untracked are causing any trouble to typescript complier.

@Marvin9
Copy link
Member

Marvin9 commented Apr 11, 2020

Code readability. I would prefer components defined as Component: React.FC<Props>

@Paarmita
Copy link
Member

Changed all default exports to constants

@Dishebh
Copy link
Member

Dishebh commented May 23, 2020

@divyanshu-rawat can I take up the issue, if it is available under gssoc?

@github-actions
Copy link

Stale issue message

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion enhancement New feature or improvement of existing feature. migration Migration from existing stack to new stack. no-issue-activity
Projects
None yet
Development

No branches or pull requests

4 participants