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

revert: back to using vue.d.ts instead of .d.ts for Vue declarations #410

Merged
merged 1 commit into from
Aug 18, 2022

Conversation

agilgur5
Copy link
Collaborator

This reverts commit e145d0b / PR #336
NOTE: this may be considered breaking, so we may want to release this in a breaking release as such. I've left this PR as draft as such. That being said, this could also be considered as just a bugfix as the original PR was made in error and caused a bug.

Summary

Revert back to using .vue.d.ts for Vue declarations

Details

  • Per follow-up discussion on the original issue and to-be-reverted PR, it seems that the request to use .d.ts instead of .vue.d.ts was made in error
    • .d.ts seems to only be necessary if Vue users were importing .vue SFCs without extensions ("extensionless")
      • i.e. import MyComponent from "./MyComponent" instead of import MyComponent from "./MyComponent.vue"
    • and "extensionless" imports are no longer supported by the Vue team (but used to be)
    • requiring extensionless imports also breaks imports when extensions are used
    • so these are not necessarily compatible with each other, but the Vue team support strongly suggests that .vue.d.ts would be the proper way forward

Credits

Thanks to @andrew0 for finding this problem and providing evidence 🙂

This reverts commit e145d0b.

- Per discussion on the original issue and reverted PR, it seems that the request to use `.d.ts` instead of `.vue.d.ts` was made in error
  - `.d.ts` seems to only be necessary if Vue users were importing `.vue` SFCs without extensions ("extensionless")
    - i.e. `import MyComponent from "./MyComponent"` instead of `import MyComponent from "./MyComponent.vue"`
  - and "extensionless" imports are no longer supported by the Vue team (but used to be)
  - requiring extensionless imports also breaks imports when extensions _are_ used
  - so these are not necessarily compatible with each other, but the Vue team support strongly suggests that `.vue.d.ts` would be the proper way forward
@agilgur5 agilgur5 added kind: bug Something isn't working properly scope: integration Related to an integration, not necessarily to core (but could influence core) labels Aug 17, 2022
@agilgur5 agilgur5 requested a review from ezolenko August 17, 2022 20:54
@ezolenko
Copy link
Owner

Yeah, we should support official way of importing, so looks good.

Do we want to release this sooner than later? Any other PRs you want to include (or exclude)?

@agilgur5
Copy link
Collaborator Author

agilgur5 commented Aug 18, 2022

Do we want to release this sooner than later? Any other PRs you want to include (or exclude)?

So depends if we're considering this as a breaking change or not.
If it's a patch, then I think this PR can go out with all the other already merged changes into 0.32.2.
If we're considering this as breaking, then it probably makes sense to release along with #406 . So then I'd think to release 0.32.2 with the already merged changes and then this PR and #406 as part of 0.33.0.

Exclude-wise, I was just thinking of leaving #406 until either 0.32.3 or 0.33.0 (pending patch/breaking classification), and releasing 0.32.2 on its own without #406. Just because #406 is a big change and there's enough changes already merged in for a sizeable release.

The rest of my PRs in the queue should not affect the release one way or another as they're only testing changes, clean-up, or refactors; i.e. no functional changes to the plugin itself.

@agilgur5 agilgur5 marked this pull request as ready for review August 18, 2022 19:23
@agilgur5 agilgur5 added kind: regression Specific type of bug -- past behavior that worked is now broken and removed kind: bug Something isn't working properly labels Aug 18, 2022
@ezolenko ezolenko merged commit bc01134 into ezolenko:master Aug 18, 2022
@ezolenko
Copy link
Owner

I think we should do 0.33 then without #406, and then 0.34 with that when it is ready

@ezolenko
Copy link
Owner

ezolenko commented Aug 19, 2022

@agilgur5 0.33.0 is out, maybe check if release notes make sense and edit them if you want anything changed

@agilgur5
Copy link
Collaborator Author

agilgur5 commented Aug 24, 2022

I think we should do 0.33 then without #406, and then 0.34 with that when it is ready

Ok, up to you 👍 Your releases consistently err on the side of breaking (vs. patch), so that's maybe more expected for rpt2 users too, in that sense.

@agilgur5 0.33.0 is out, maybe check if release notes make sense and edit them if you want anything changed

Thanks for (mostly) copying my release notes style!
I made a few adjustments, for reference:

  • put "Optimizations", "Docs", and "GitHub" sections outside of the "Other"/"Internal" section as they are user-facing and so may be good to highlight
  • put this PR, which begins with revert: into the "Bugfixes" section
  • put the dx: PRs into the "Docs" section (that's kind of the closest thing to it)
  • highlighted a few of the "Internal" changes before the spoiler too -- think users would be happy to see all the testing additions at least!
  • hid a few "Bugfixes" etc under a spoiler where they were not user-facing or very minor in nature
  • added explanations to several of the "Bugfixes" where the PR title would not necessarily be understood by users

I also saw that the dist/ build was missing in the release initially, then saw that you re-tagged it. Looks like NPM has the build already, so seems all good minus some tag confusion 👍

@ezolenko
Copy link
Owner

I basically took autogenerated release notes with your commit messages and rearranged them to hide internal stuff, so those are literally your release notes. :)

Yeah, forgot to push local branch with build commit before tagging on github :/

@agilgur5
Copy link
Collaborator Author

I basically took autogenerated release notes with your commit messages and rearranged them to hide internal stuff, so those are literally your release notes. :)

Yep that's the basic gist of the structure I use 👍 plus a few manual things where explanations could be helpful

Yeah, forgot to push local branch with build commit before tagging on github :/

Yea I've done that multiple times before too 😅

If you're interested, we can also automate the release process with GH Actions too

@agilgur5 agilgur5 added the scope: vue Related to integration with Vue (rollup-plugin-vue is long archived), not core label Mar 14, 2023
@agilgur5 agilgur5 deleted the revert-vue-d-ts branch July 2, 2023 21:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: regression Specific type of bug -- past behavior that worked is now broken scope: integration Related to an integration, not necessarily to core (but could influence core) scope: vue Related to integration with Vue (rollup-plugin-vue is long archived), not core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants