Skip to content

chore: remove duplicated logic in vanilla template#10145

Merged
patak-cat merged 3 commits intovitejs:mainfrom
curran:patch-1
Sep 22, 2022
Merged

chore: remove duplicated logic in vanilla template#10145
patak-cat merged 3 commits intovitejs:mainfrom
curran:patch-1

Conversation

@curran
Copy link
Copy Markdown
Contributor

@curran curran commented Sep 16, 2022

Description

When getting started with Vite, I noticed some odd code in the vanilla template:

export function setupCounter(element) {
  let counter = 0              
  const setCounter = (count) => { 
    counter = count            
    element.innerHTML = `count is ${counter}`
  }
  element.addEventListener('click', () => setCounter(++counter))
  setCounter(0)                
} 

The ++counter expression actually mutates counter, but then it is also mutated by counter = count. I find this confusing, and it's not the sort of thing I would expect to see in a template example.

I would suggest to make a small change so that instead of mutating counter twice, it's mutated only once, inside the body of setCounter.

Additional context


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the Commit Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@sapphi-red sapphi-red added the p1-chore Doesn't change code behavior (priority) label Sep 18, 2022
@sapphi-red sapphi-red changed the title Remove duplicated logic in vanilla example refactor: remove duplicated logic in vanilla template Sep 18, 2022
Copy link
Copy Markdown
Member

@sapphi-red sapphi-red left a comment

Choose a reason for hiding this comment

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

Thanks! Would you update this line, too?

element.addEventListener('click', () => setCounter(++counter))

@curran
Copy link
Copy Markdown
Contributor Author

curran commented Sep 21, 2022

Will do! Thanks for reviewing.

@curran
Copy link
Copy Markdown
Contributor Author

curran commented Sep 21, 2022

Done. Requesting re-review please @sapphi-red. Thanks!

@curran curran requested a review from sapphi-red September 21, 2022 19:22
@patak-cat patak-cat changed the title refactor: remove duplicated logic in vanilla template chore: remove duplicated logic in vanilla template Sep 22, 2022
@patak-cat patak-cat merged commit f29fef5 into vitejs:main Sep 22, 2022
@patak-cat
Copy link
Copy Markdown
Member

Thanks @curran, good catch!

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

Labels

p1-chore Doesn't change code behavior (priority)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants