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

chore: organize codebase #313

Merged
merged 13 commits into from Aug 19, 2023
Merged

chore: organize codebase #313

merged 13 commits into from Aug 19, 2023

Conversation

mukundshah
Copy link
Contributor

  • move eslint to root of project and update the config to use @antfu/eslint-config
  • add vscode specific settings and extension recommendation
  • update PR and Issues template
  • perform some miscellaneous cleanup

@zernonia zernonia self-requested a review August 19, 2023 06:52
Copy link
Collaborator

@zernonia zernonia left a comment

Choose a reason for hiding this comment

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

let's remove other "eslint config" file in radix-vue.
image

some minor suggestion, else lgtm!!!

@zernonia zernonia merged commit 87ca501 into radix-vue:main Aug 19, 2023
2 of 3 checks passed
zernonia added a commit that referenced this pull request Aug 19, 2023
zernonia added a commit that referenced this pull request Aug 19, 2023
@zernonia
Copy link
Collaborator

@mukundshah Thanks for the PR. However I only realized this changes broke the whole app, especially related to radix-vue package. If you look at any root component, you can see error TS1184: Modifiers cannot appear here., thus I've reverted it.

Could you look into that please? 😁

@mukundshah
Copy link
Contributor Author

@zernonia
Copy link
Collaborator

@zernonia <script setup> could not contain exports.

Reference: eslint.vuejs.org/rules/no-export-in-script-setup.html https://github.com/vuejs/rfcs/blob/master/active-rfcs/0040-script-setup.md

hmm.. but it's not only in script setup, also in normal script tag. This changes kinda messed up the codebase, and had to refactor everything (which is not a good idea).

@Blinks44
Copy link

Is not exports in script-setup has been fixed in vue 3.3+?
vuejs/core#7794

@mukundshah
Copy link
Contributor Author

@Blinks44 thank you for bringing that rfc tto attenttion.

@zernonia i studied the issue, and conducted some experiments as well.

The conclusion is:

Both the editor and vue-tsc should have thrown error/warnings at the very beginning. I reverted every change I made, and I got red everywhere it's supposed to. Thus, the final conclusion is a refactor.

@zernonia
Copy link
Collaborator

zernonia commented Aug 21, 2023

@Blinks44 thank you for bringing that rfc tto attenttion.

@zernonia i studied the issue, and conducted some experiments as well.

The conclusion is:

Both the editor and vue-tsc should have thrown error/warnings at the very beginning. I reverted every change I made, and I got red everywhere it's supposed to. Thus, the final conclusion is a refactor.

I think you missed my pointer earlier @mukundshah .
I know script setup only export ineterface/types, but the script tag should be able to export const, as it acts like a normal js file.

image

I know the syntatic sugar script setup has it's limitation, but using script itself works pretty well, and it does all the job we want it to do. This definitely not called for a refactor. And I hope that you understand, this refactor is super unncessary if we dont have a clear and heavily favoured benefit that it will bring.

Furthermore, I found support for defineComponent is quite limitated with TS. You can checkout the TS for props/emits defined in each component in Headless UI. All these is not not neccessary with the current .vue SFC approach that we have.

You might argue that >3.3 we can have better types for defineComponent, yes you are right, but it's still limited as we still need to manually define the emits in each component.

@zernonia
Copy link
Collaborator

zernonia commented Aug 21, 2023

Also, I've found a way to use antfu/eslint, by having the following config. Doing so we prevent all exports being moved to script setup, which prevent us from having those export const issue.

eslint-plugin-vue currently have no active way to distinguish script & script setup due to how it was wired in vue/core. So we can just disable this eslint hint for our repo.

Reference: vuejs/eslint-plugin-vue#1577

module.exports = {
  extends: '@antfu',

  rules: {
    'import/first': 'off',
  },
}

Note:

  • all types work correctly with script tag exporting const
    image

  • all types works correctly with importing the exported const
    image

@mukundshah
Copy link
Contributor Author

I know script setup only export ineterface/types, but the script tag should be able to export const, as it acts like a normal js file.

@zernonia we still need to refactor the code base. Those exports need to be arranged. Isn't it?

@zernonia
Copy link
Collaborator

I know script setup only export ineterface/types, but the script tag should be able to export const, as it acts like a normal js file.

@zernonia we still need to refactor the code base. Those exports need to be arranged. Isn't it?

Nope. Not needed 😁

@zernonia zernonia mentioned this pull request Aug 21, 2023
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.

None yet

3 participants