Skip to content

react-noop-renderer: convert vars into let/const#11732

Closed
raphamorim wants to merge 2 commits intofacebook:masterfrom
raphamorim:react-noop-renderer/no-more-vars
Closed

react-noop-renderer: convert vars into let/const#11732
raphamorim wants to merge 2 commits intofacebook:masterfrom
raphamorim:react-noop-renderer/no-more-vars

Conversation

@raphamorim
Copy link
Copy Markdown
Contributor

ref: #11730
last one @gaearon, I promise 😆

@gaearon
Copy link
Copy Markdown
Collaborator

gaearon commented Dec 1, 2017

Was react-dom fully converted? Pretty sure I saw some vars there..

@blling
Copy link
Copy Markdown

blling commented Dec 1, 2017

How to prevent developer from use var again?

@gaearon
Copy link
Copy Markdown
Collaborator

gaearon commented Dec 1, 2017

We could lint against it but this is a bit tricky because some files have to use var. I think ideally we should have a way to override lint configuration for a subset of files. We have the same problem in Prettier config (but solved it there).

@raphamorim
Copy link
Copy Markdown
Contributor Author

raphamorim commented Dec 1, 2017

Hm. In React-DOM, I keep some vars because of some SSR tests are using DEV blocks (I believe this is the reason)

@gaearon
Copy link
Copy Markdown
Collaborator

gaearon commented Dec 1, 2017

Can you explain that in more detail?

@raphamorim
Copy link
Copy Markdown
Contributor Author

One sec. I'll give a example


if (__DEV__) {
var validatePropertiesInDevelopment = function(type, props) {
let validatePropertiesInDevelopment = function(type, props) {
Copy link
Copy Markdown
Contributor Author

@raphamorim raphamorim Dec 1, 2017

Choose a reason for hiding this comment

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

Hm, @gaearon I guess my first idea is wrong. Seems to be a scope issue.

In this case I simple changed it and returns errors like:
ReferenceError: validatePropertiesInDevelopment is not defined on tests.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Yep—you can define it outside the dev block.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

(but assign it inside)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Maybe let validatePropertiesInDevelopment = () => {}; would be the best default value, and then in the block you reassign.

Copy link
Copy Markdown
Contributor Author

@raphamorim raphamorim Dec 1, 2017

Choose a reason for hiding this comment

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

ok, I'll fix for react-dom, and see if forgot some var in other package then. I'll add on this PR.

Copy link
Copy Markdown
Contributor Author

@raphamorim raphamorim Dec 1, 2017

Choose a reason for hiding this comment

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

It's okay for you?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think it's better I close this PR and send one with another branch name :P

@raphamorim raphamorim closed this Dec 1, 2017
@raphamorim raphamorim deleted the react-noop-renderer/no-more-vars branch December 1, 2017 01:12
@raphamorim raphamorim mentioned this pull request Dec 1, 2017
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants