Skip to content

refactor: simplify the code #91

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

Merged
merged 1 commit into from
Jan 18, 2022
Merged

refactor: simplify the code #91

merged 1 commit into from
Jan 18, 2022

Conversation

lomirus
Copy link
Contributor

@lomirus lomirus commented Oct 5, 2021

It seems strange to write assignment in the if statement, and to use the let token instead of const to define a variable that you will never change. I just started reading the source code of this project recently, so I tried to simplify the code to make some refinements on the entry file.

@JSerFeng
Copy link

JSerFeng commented Oct 6, 2021

I think it's just Evan's style maybe, you can find a lot similar code in vue as well.

Comment on lines +7 to 10
const s = document.currentScript
if (s && s.hasAttribute('init')) {
createApp().mount()
}
Copy link

@Amar-Gill Amar-Gill Oct 15, 2021

Choose a reason for hiding this comment

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

Suggested change
const s = document.currentScript
if (s && s.hasAttribute('init')) {
createApp().mount()
}
document.currentScript?.hasAttribute('init') && createApp().mount()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Elegant code! But do we really need the question mark after the document?

Copy link

@Amar-Gill Amar-Gill Oct 24, 2021

Choose a reason for hiding this comment

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

Maybe -- I'm replicating the logic. if s = document.currentScript and then on line 8, we have s && ... I imagine document.currentScript could be undefined. Don't have any experience using this property however.

Choose a reason for hiding this comment

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

But then we only need document.currentScript?.someMethod as document will always be defined in this context.

Choose a reason for hiding this comment

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

revised

@dongwa
Copy link

dongwa commented Jan 7, 2022

It seems strange to write assignment in the if statement, and to use the let token instead of const to define a variable that you will never change. I just started reading the source code of this project recently, so I tried to simplify the code to make some refinements on the entry file.
好家伙,您就是葫芦娃中的水娃吧

@lomirus
Copy link
Contributor Author

lomirus commented Jan 7, 2022

好家伙,您就是葫芦娃中的水娃吧

But I just thought the readability is also part of the enhancement right? 😂

@yyx990803 yyx990803 merged commit 86681e2 into vuejs:main Jan 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants