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

Dependencies: Use latest version of vue-tsc & sync versions of angular #23011

Merged
merged 5 commits into from
Jun 9, 2023

Conversation

ndelangen
Copy link
Member

@ndelangen ndelangen commented Jun 9, 2023

some volar update caused vue to start vomitting ts-errors

It seems that when a file file filename Button.vue is used, it will define a component button.
I'm not sure of this is a bug or a feature.

But if Button.vue uses <button> (which would obviously refer to HTMLButtonElement), volar gets "confused" and assumes thus button is the button being defined.

But of course the API (props, events) of those 2 are different, thus it complains.

I renamed Button.vue to MyButton.vue, this fixes it.

I'm not really happy with this though.

@ndelangen ndelangen self-assigned this Jun 9, 2023
@ndelangen ndelangen added the build Internal-facing build tooling & test updates label Jun 9, 2023
Base automatically changed from norbert/upgrades-testing to next June 9, 2023 14:15
code/yarn.lock Outdated Show resolved Hide resolved
code/yarn.lock Outdated Show resolved Hide resolved
…ilenames are treated to override native html elements, causing types clashes.
…ned by filenames are treated to override native html elements, causing types clashes."

This reverts commit ea7c392.
@ndelangen
Copy link
Member Author

ndelangen commented Jun 9, 2023

This seems related:
#22853 (comment)

Seems @volar/typescript is on npm on the latest tag with an actual prerelease?!?

(norbert/sync-package-versions-angular) [1] % npm view @volar/typescript                                       ~/Projects/Storybook/core/code

@volar/typescript@1.7.4 | MIT | deps: 1 | versions: 91
https://github.com/volarjs/volar.js#readme

dist
.tarball: https://registry.npmjs.org/@volar/typescript/-/typescript-1.7.4.tgz
.shasum: b323d57c9d9a17cb77008455a2c95dc6c36fabe7
.integrity: sha512-xHiQoKAwXdLt1NXYZZ3uD32PKJ8KpZ5raBU6uCgCbhUh+7uWsk+cyFqDAboAPPm3oBCyCEGnnbrsJvimPEWyrA==
.unpackedSize: 115.2 kB

dependencies:
@volar/language-core: 1.7.4 

maintainers:
- johnsoncodehk <johnsoncodehk@gmail.com>

dist-tags:
latest: 1.7.4        next: 1.5.0-alpha.0  

https://github.com/vuejs/language-tools/blob/master/CHANGELOG.md

I'll downgrade back to what they deem to be latest and lock it.
Why aren't they publishing prereleases as a separate tag?

@ndelangen ndelangen changed the title Build: Sync versions of angular Dependencies: Use latest version of vue-tsc & sync versions of angular Jun 9, 2023
@ndelangen ndelangen added dependencies and removed build Internal-facing build tooling & test updates labels Jun 9, 2023
@socket-security
Copy link

New and updated dependency changes detected. Learn more about Socket for GitHub ↗︎

Packages Version New capabilities Transitives1 Size Publisher
vue-tsc ⬆️ 1.7.11...1.6.5 None +12/-12 616 kB johnsoncodehk

Footnotes

  1. https://docs.socket.dev

@ndelangen
Copy link
Member Author

@Sidnioulz here's a PR of us switching us to the "latest" release according to their CHANGELOG.

Do you expect this will cause issues for storybook users?
Or will it actually fix issues?

Copy link
Contributor

@kasperpeulen kasperpeulen left a comment

Choose a reason for hiding this comment

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

I think this is great, let's in an other PR also put vue-component-type-helpers to 'latest' to fix the issues of @Sidnioulz

@ndelangen ndelangen merged commit 12295b7 into next Jun 9, 2023
@ndelangen ndelangen deleted the norbert/sync-package-versions-angular branch June 9, 2023 16:15
@Sidnioulz
Copy link
Contributor

@ndelangen sorry, mail goes to my personal inbox and it takes time to process :(

I don't expect any change or improvement with vue-component-type-helpers. In my limited Vue experience, the issue you describe is bound to happen because of single-component naming.

It's a convention in Vue that component names must be multi-word. The Vue maintainers provide an ESLint rule to enforce that: https://eslint.vuejs.org/rules/multi-word-component-names.html.

Without enforcing the rule, we've seen other linters in the past complain about Button.vue and whatnot not respecting the APIs of their HTML standard counterparts. The expectation is so strong in the Vue community that we just accept it and use a prefix on our Vue component names.

Anyone struggling with this and wanting to keep their file named Button.vue can define the name explicitly:

// Vue 3 composition API
defineOptions({
  name: 'MyButton',
})

// Vue 3 options API, never used it but I guess it's that
export default {
  name: 'MyButton',
  ...
}

// Vue 2 with property decorators / class components
@Component
export default class MyButton extends Vue { ... }

@ndelangen
Copy link
Member Author

Good to know @Sidnioulz, to the problem I faced, will come back to haunt us later.. And might be actually hunting users of storybook currently?

We should perhaps setup this mentioned eslint rule on the vue part of our codebase.

Any help would be appreciated @Sidnioulz if you have the time 🙏
I know @chakAs3 and @kasperpeulen are the resident vue-experts, they might be keen to know this as well: #23011 (comment)

@chakAs3
Copy link
Contributor

chakAs3 commented Jun 20, 2023

Hi @ndelangen indeed Vue uses and recommends using PascalCase names because they are valid JavaScript identifiers so easy to register and import, and easy for IDE to auto-complete. it distinguishes the Vue component from a native or custom component (web component).

However, Vue can resolve to kebab-case as well so both <MyComponent/> and <my-component/> are valid.
the take from this is we should avoid Naming our Component name that can resolve to a native Element ex: <Button /> => <button />, <Table /> => <table />, the solution that comes to mind is PascalCase, which resolves to and as we know there is no native element in kebab case.

Nonetheless renaming Button.vue to MyButton.vue may not be the best thing to do, i think it is about the default export.
so for me, we should more do import MyButton from 'Button.vue'

@Sidnioulz
Copy link
Contributor

Sidnioulz commented Jun 21, 2023

Good to know @Sidnioulz, to the problem I faced, will come back to haunt us later.. And might be actually hunting users of storybook currently?

It's possible. I think my first issue with this was SB's a11y linter throwing incorrect warnings.

We should perhaps setup this mentioned eslint rule on the vue part of our codebase.

I'm tempted to say yes, but if some engineers insist on using the single-word names in their codebases, it would cause significantly more annoyance to them. So, no opinion.


The solution that comes to mind is PascalCase, which resolves to and as we know there is no native element in kebab case.

We used to have PascalCase and it was not enough. After all, HTML tag and attribute names are case insensitive. button is BUTTON or Button or bUtToN. Many tools will normalise case before running whatever they want to, because of that.

Nonetheless renaming Button.vue to MyButton.vue may not be the best thing to do, i think it is about the default export. so for me, we should more do import MyButton from 'Button.vue'

This may work for components that are globally registered. If components aren't globally registered, but passed via the components option, then the name you use to import them must match the name option (can't explain why any more, but that caused bugs somewhere). In our codebase, we kept our file names intact but added a prefix to names everywhere, and we force engineers to use the prefix in their import name.

@chakAs3
Copy link
Contributor

chakAs3 commented Jun 21, 2023

@Sidnioulz yeah right it is really upto the developers if they want to pay it save and clean, despite the tools and rules that help have a saint codebase you can always screw it up believe me.

but passed via the components option, then the name you use to import them must match the name option (can't explain why any more, but that caused bugs somewhere)

Yes, but to be honest I don't do much code using options to define. the component name unless you really want to be explicit about the component name, this is most for Ui libraries.
For me, in certain cases, the component does not play a big role in my code, within my work on Volar component meta I was doing something like this for tests.
image

I could be a bit more specific and just change the import name

import type { StoryObj, Meta } from '@storybook/vue3';

import SlotsComponent from './template-slots/component.vue';

const meta = {
  component: SlotsComponent,
  tags: ['autodocs'],
} satisfies Meta<typeof SlotsComponent>;
type Story = StoryObj<typeof meta>;
export default meta;

export const Default: Story = {
  args: {
    default: ({ num }) => `Default slot { num=${num} }`,
    named: ({ str }) => `Named slot { str=${str} }`,
    vbind: ({ num, str }) => `Named v-bind slot { num=${num}, str=${str} }`,
  },
};

so my take is you can use generic name if the scope is local and not impacting any part of your codebase, the minute your component exposed to other part of the codebase you should be careful About The sematic and the format of your component name and follow the team / framework conventions

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