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

refactor: mutate config in normalizeConfig and handleLocalBackend fns #4846

Merged

Conversation

smashercosmo
Copy link
Contributor

Summary

Another atomic PR to make #4776 easier to merge

Test plan

Run current tests

@smashercosmo smashercosmo requested a review from a team January 18, 2021 12:18
@erezrokah erezrokah added the type: chore work needed to keep the product and development running smoothly label Jan 18, 2021
@smashercosmo smashercosmo force-pushed the mutate-normalize-handle-local-backend branch from 66021ef to 79bc07e Compare January 18, 2021 13:17
@smashercosmo smashercosmo force-pushed the mutate-normalize-handle-local-backend branch 3 times, most recently from 9c3c997 to cc0bd0e Compare January 25, 2021 16:32
@smashercosmo
Copy link
Contributor Author

Seems like latest commits from master cause e2e tests to fail

@erezrokah
Copy link
Contributor

Seems like latest commits from master cause e2e tests to fail

👀

@smashercosmo smashercosmo force-pushed the mutate-normalize-handle-local-backend branch from ca4add6 to 54789f2 Compare January 26, 2021 13:18
@smashercosmo
Copy link
Contributor Author

Hm... now I have no idea why it fails. When I run e2e tests locally they pass.

@smashercosmo
Copy link
Contributor Author

@erezrokah I think I'm gonna need your help with it ((

@erezrokah
Copy link
Contributor

Sorry for the delay @smashercosmo. I'm clearing some stuff off my plate and we'll review the PRs this week.

@erezrokah erezrokah force-pushed the mutate-normalize-handle-local-backend branch from 54789f2 to 353dbd9 Compare January 29, 2021 10:40
@erezrokah
Copy link
Contributor

@erezrokah
Copy link
Contributor

I'm going to try and revert the Cypress version per cypress-io/cypress#14663

@erezrokah erezrokah force-pushed the mutate-normalize-handle-local-backend branch from 353dbd9 to a05dc3a Compare January 29, 2021 14:06
@erezrokah erezrokah force-pushed the mutate-normalize-handle-local-backend branch from a05dc3a to e74a152 Compare February 7, 2021 14:22
@erezrokah erezrokah force-pushed the mutate-normalize-handle-local-backend branch from 6de7f8c to bd31c24 Compare February 7, 2021 15:28
Copy link
Contributor

@erezrokah erezrokah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay @smashercosmo, thank you for another great PR which makes our codebase much cleaner and easier to understand.

I decided to keep the methods non-mutating as personally I believe they make the code easier to follow.

@erezrokah erezrokah merged commit 3924cfb into decaporg:master Feb 7, 2021
@smashercosmo
Copy link
Contributor Author

hm... @erezrokah I understand you concern, but if you want to keep methods immutable, I would suggest we just use immer instead of object spreads.

@erezrokah
Copy link
Contributor

hm... @erezrokah I understand you concern, but if you want to keep methods immutable, I would suggest we just use immer instead of object spreads.

That's a good point. I wasn't sure if the the nesting was deep enough to justify that.

@smashercosmo
Copy link
Contributor Author

@erezrokah another question not directly related to this PR but to netlify-cms code style in general. I noticed that you're using both function expressions and function declarations across codebase, but seems like there is no logic in when either is being used. I would propose to use function expressions only in inline callbacks and declarations in all other cases. Because it's easier to distinguish function from a simple variable when function is declared as declaration.
Bad:
const someVar = 1
const someFn = () => { ... }

Good:
const someVar = 1
function someFn() { ... }

@erezrokah
Copy link
Contributor

Another good point. We mostly care about consistency (which we don't have).
What do you think about adding https://eslint.org/docs/rules/func-style to our linting configuration?

@smashercosmo
Copy link
Contributor Author

Well, surely I can, but should I then fix all of the eslint errors? 🙂

@erezrokah
Copy link
Contributor

erezrokah commented Feb 7, 2021

I'm mostly interested on how many errors we get :) we can apply the rule for specific parts of the codebase and slow fix errors, or add it as a warning?

@smashercosmo
Copy link
Contributor Author

618 errors 🙂 I'll check if it can be fixed by some codemod

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: chore work needed to keep the product and development running smoothly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants