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

feat: support Nuxt 3 and Bridge (wip) #396

Closed
wants to merge 1 commit into from
Closed

feat: support Nuxt 3 and Bridge (wip) #396

wants to merge 1 commit into from

Conversation

NozomuIkuta
Copy link

@NozomuIkuta NozomuIkuta commented Mar 1, 2022

⚠️ This PR is still in progress ⚠️

Resolves nuxt/nuxt#11030
Resolves nuxt/framework#387

TODOs

  • Scaffold by Nuxt's module starter
  • Implement basic features
    • Plugin injection (Nuxt 3)
    • Plugin typings (Nuxt 3)
    • Plugin injection (Nuxt Bridge)
    • Plugin typings (Nuxt Bridge) (Help wanted, see below)
    • Module options to dynamically set up Day.js on runtime (i.e. dayjs.config.mjs)
      • locales option
      • defaultLocale option
      • plugins option
      • defaultTimeZone option
  • Reimplement unit tests (awaiting for Test utils support progress nuxt/nuxt#13372 to use Vitest)
  • Reimplement e2e tests (awaiting for Test utils support progress nuxt/nuxt#13372)
  • Restore other kind of files (e.g. LICENCE file)
    • I deleted these files by scaffolding, and I will restore them after completing essential parts and before making this PR ready for review

Help Wanted: Plugin Typings for Nuxt Bridge

While plugins for Nuxt 3 defined by defineNuxtPlugin() are perfectly type-safe, Nuxt Bridge doesn't because it uses traditional export default function () style.

So, I added the reference to the code below to .nuxt/nuxt.d.ts via prepare:types hook.

import dayjs from 'dayjs'

declare module 'vue/types/vue' {
  interface Vue {
    $dayjs: typeof dayjs
  }
}

declare module '@nuxt/types' {
  interface NuxtAppOptions {
    $dayjs: typeof dayjs
  }

  interface Context {
    $dayjs: typeof dayjs
  }
}

declare module 'vuex/types/index' {
  interface Store<S> {
    $dayjs: typeof dayjs
  }
}

With the code above, I confirmed $dayjs is inferred:

<template>
  {{ $dayjs }} <!-- VSCode says $dayjs: typeof dayjs -->
</template>

<script>
import Vue from 'vue'

export default Vue.extend({
  asyncData(ctx) {
    ctx.$dayjs() // VSCode says $dayjs: typeof dayjs
  }
})
</script>

However, the type is not inferred in setup script:

<script setup lang="ts">
const nuxtApp = useNuxtApp() // VSCode says nuxtApp: any
nuxtApp.$dayjs() // VSCode says $dayjs: any
</script>

If I'm correct, useNuxtApp() returns an object of type NuxtAppCompat, and I can't add a type to it because this is not exported from Nuxt Bridge. Am I missing something?

(ref. original message posted to Discord)


@pi0 @Atinux @danielroe

Let me mention you guys for help, because you are definitely best at Nuxt development. 🙏

Copy link
Member

Atinux commented Mar 1, 2022

I would go first with Nuxt 3 support without bridge to iterate faster @NozomuIkuta

Thank you so much for you help ❤️

@danielroe
Copy link
Member

For now, it should be safe to augment from #app, and when we resolve nuxt/bridge#91 it will then be type hinted...:

declare module '#app' {
  interface NuxtAppCompat {
    // ...
  }
}

I would suggest exposing a useDayjs composable that is typed, which will work everywhere, and maybe even turning off the default $dayjs injection (that is, require an opt-in). That will allow for better code optimisation, because we don't need to run any setup.

If all that is exposed is dayjs itself, we could likely also just expose it as an auto-imported composable rather than inject it globally.

@NozomuIkuta
Copy link
Author

@danielroe

Thank you for the feedback!

I will check out how core modules do auto-importing composables. 🙋‍♀️

I used injection just to provide the same DX as the current version. But, yeah, now it's time to move to composables with optional plug-in injection.

Copy link
Member

@potato4d potato4d left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution!
I'm looking forward to completing my pull request ;)

@potato4d potato4d self-assigned this Mar 7, 2022
@potato4d
Copy link
Member

potato4d commented Mar 7, 2022

It appears as if the project is being rebuilt from scratch.
When you remove the WIP, please restore all elements correctly, including CircleCI settings and README.

Also, from a review point of view, please only address Nuxt3 in this pull request, and do the introduction of Vitest, etc. in other pull requests.

I hope that you will not rework the project at your own discretion, but will correctly revise the code base so far!

thank you!

@NozomuIkuta
Copy link
Author

NozomuIkuta commented Mar 7, 2022

@potato4d

It appears as if the project is being rebuilt from scratch.
When you remove the WIP, please restore all elements correctly, including CircleCI settings and README.

Yes, of course I will restore the existing code base before I make this PR ready for review.
The files are temporarily deleted just because of scaffolding for Nuxt 3 module, as described in PR message.

I created this draft PR mainly to notify people that "I'm working on Nuxt 3 support", which I think is better rather than silent development.

As for testing, I didn't plan migration to Vitest just because I want to use it. I did because I found Nuxt core modules are tested with Vitest and the new Test Utils, which they announced to release soon.
I thought it would be beneficial if this module is aligned with core modules in terms of the way of development.

In addition, I was not sure the current version of Nuxt Test Utils works with Nuxt 3, so I felt like waiting for the new Test Utils if releasing Nuxt-3-compatible DayJS module is not urgent.

Otherwise, if the current version of Nuxt Test Utils doesn't work with Nuxt 3, we have to release this module only with manual testing in playground directory with few unit testing (I guess, for module option validators only).
I believe E2E testing is essential for Nuxt modules that render something on the screen (or in HTML string).

Anyway, at least I agree that I should have separated tasks for Nuxt 3 and Nuxt Bridge.
Let me create GitHub issues for each and handle one by one.

Here, I would like to know situation of and your thoughts on Nuxt 3 support.

  • Is the current version of Nuxt Test Utils available (or recommended) for Nuxt 3 testing?
    • If not, should we release DayJS module with manual testing to release earlier, or should we wait for the new Nuxt Test Utils?
    • If available, I will reuse the existing e2e testing code and add testing for composition API, as @danielroe suggested.

@danielroe
Copy link
Member

Yes, the new version of nuxt test utils is working, assuming we're happy to update the test implementation as things develop. (Things might change somewhat in the API.) 👍

Here is a very basic example: nuxt-modules/harlem#8.

@NozomuIkuta
Copy link
Author

@danielroe

Thank you for the information!
I will dig into the repository. 🙋‍♀️

@NozomuIkuta
Copy link
Author

I have created nuxt/nuxt#11772 to continue discussion and clarify where we should go.
I appreciate if you take a look and leave some comments.

@NozomuIkuta
Copy link
Author

I'm sorry for inconvenience, but I have come to have to handle my personal stuff.
So, I'm going to close this PR for the time being, because keeping opening a draft PR may be confusing for other people.

I hope administrators/maintainers could reopen and take it over, or someone else could submit another PR for further development.

Thank you for taking your time and posting feedback and advice for this PR. 🙇‍♂️
I will reopen this PR if I become available again, or will find another opportunity to contribute if there would have been already a same kind of PR submitted.

@NozomuIkuta NozomuIkuta closed this Mar 9, 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.

disable moduleSideEffects for nitro bundle
4 participants