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

refactor: rewrite with typescript and module-builder #456

Merged
merged 45 commits into from
Mar 17, 2023

Conversation

rtibaldo
Copy link
Contributor

Refactored module using nuxt/module-builder (using typescript) and nuxt/kit to begin working towards nuxt 3 compatibility.

Test are semi-broken as module-builder requires nuxt 3 so they now have their own package.json with nuxt 2.
I don't know if this is the best solution, but I couldn't come up with anything else

@rchl
Copy link
Member

rchl commented Nov 14, 2022

Haven't looked at the changes yet but are you saying that it should be possible to use module-builder and nuxt/kit and at the same time still have it compatible with Nuxt 2?

@rtibaldo
Copy link
Contributor Author

Yes, we should be able to have compatibility with both nuxt 2 and 3.
The only thing that has a completely different sintax are plugins but since we are using templates for those we can circumvent that. The unocss nuxt module does that and has nuxt 2 and 3 compatibility

src/module.ts Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
src/module.ts Outdated Show resolved Hide resolved
build.config.ts Outdated Show resolved Hide resolved
.eslintrc Outdated Show resolved Hide resolved
types/sentry.d.ts Outdated Show resolved Hide resolved
@rchl rchl mentioned this pull request Nov 17, 2022
src/core/hooks.ts Outdated Show resolved Hide resolved
.eslintrc.cjs Outdated Show resolved Hide resolved
@daniluk4000
Copy link

daniluk4000 commented Nov 21, 2022

@rtibaldo I've tried to use come of your changes

"context.$sentry.setUser is not a function"

console.log(context.$sentry); result: {}

sentry.server.ts
const sentry = process.sentry || {}

Guess this is it, process.sentry is undefined

@rchl
Copy link
Member

rchl commented Nov 21, 2022

@rtibaldo I've tried to use come of your changes

On which Nuxt version? Those changes do not make module work in Nuxt 3 (at least not all functionality).

@daniluk4000
Copy link

@rtibaldo I've tried to use come of your changes

On which Nuxt version? Those changes do not make module work in Nuxt 3 (at least not all functionality).

Nuxt 2 + Nuxt Bridge

@rtibaldo
Copy link
Contributor Author

Nuxt 2 + Nuxt Bridge

My scope for this PR was to rewrite the module using nuxt module-builder without breaking Nuxt 2 functionality and then in a following PR add Nuxt 3 (and Bridge) support.

As for your issue I tried it locally upgrading the default test app to Bridge and I think the issue is on the client side and not on the server as this was the 'test' result
image

@rtibaldo
Copy link
Contributor Author

Test are semi-broken as module-builder requires nuxt 3 so they now have their own package.json with nuxt 2.
I don't know if this is the best solution, but I couldn't come up with anything else

So I've never worked with a monorepo before (and I could be saying something stupid) but wouldn't using one be the best long term solution to handle testing different versions of Nuxt?

Wdyt @rchl?

@rtibaldo
Copy link
Contributor Author

I dug up what the issue was and found a workaround for the problem I was having with Bridge, maybe it would resolve also yours @daniluk4000.

It seems like there is a problem with the esm version of tslib that gives an error when importing on the client side.
image

I found this issue on the Nuxt Bridge repo where @danielroe gave a workaround and explained why there is this error.

TL DR: Add this to your nuxt config

  alias: {
    tslib: 'tslib/tslib.es6.js'
  },

PS. This does not mean the module is Bridge compatible, there are things that I'm pretty sure don't work, like Nitro errors being traced

@daniluk4000
Copy link

@rtibaldo looks interesting, I'll try that tomorrow, thanks

@rchl
Copy link
Member

rchl commented Nov 22, 2022

Test are semi-broken as module-builder requires nuxt 3 so they now have their own package.json with nuxt 2.
I don't know if this is the best solution, but I couldn't come up with anything else

So I've never worked with a monorepo before (and I could be saying something stupid) but wouldn't using one be the best long term solution to handle testing different versions of Nuxt?

I don't necessarily see a need for monorepo just yet.
I think the main issue with the test setup right now is that the nuxt2 directory extends its tsconfig.json (https://github.com/rtibaldo/sentry-module/blob/daa948ca698a567309e7727d48f76d3e24a616ba/test/tsconfig.json#L2-L2) with the one generated by Nuxt 3 playground and that is not gonna work. It should just have its own, independent config.

@BlakeB415
Copy link

BlakeB415 commented Nov 28, 2022

Nuxt v3 is able to run in Cloudflare Workers. Will this be compatible in that context?

There's a sentry module made by Cloudflare here if that's any help. https://github.com/cloudflare/worker-sentry

@rchl
Copy link
Member

rchl commented Dec 12, 2022

I think we're gonna handle upgrade to v7 SDK first (#461), release as new major version and then pick this up. This will also likely have to be released as yet another major version.

@rchl
Copy link
Member

rchl commented Mar 6, 2023

I've released the current state as @nuxtjs/sentry@beta (@nuxtjs/sentry@8.0.0-beta.2).

Testing likes to crash for some reason on CI. Works locally (usually).

@rchl
Copy link
Member

rchl commented Mar 7, 2023

A bit late but just noticed another deal breaker. @nuxt/kit depends on @nuxt/schema which depends on vue 3 which completely breaks types when using beta release of this module in a Nuxt 2 project...

(EDIT: Not only Vue 2 but also other Nuxt-3 only packages)

@danielroe
Copy link
Member

Just checking, @nuxt/kit and @nuxt/schema do not depend on Vue 3 at all. They will use types from it if it is installed in your project. Rather, the issue I think you might be encountering is this one: nuxt/nuxt#19526. I've been able to type check the prerelease with no issues once I make a slight tweak to @nuxt/types.

@rchl
Copy link
Member

rchl commented Mar 9, 2023

the issue I think you might be encountering is this one: nuxt/nuxt#19526.

That's one of the issues that we've discussed in private.

The other issue has to do with using @nuxt/module-builder which automatically generates module schema for the module. That results in @nuxt/schema being referenced in the resulting type definitions (module.d.ts in our case).

Specifically this line:

import * as _nuxt_schema from '@nuxt/schema';

You've provided a possible workaround by casting module export to any like:

export default defineNuxtModule(...) as any

and it does in fact get rid of @nuxt/schema references but I'm not gonna believe that this is the official way to do it. It kinda defeats the purpose of using @nuxt/module-builder in the first place. I could instead use some other typescript compiler unrelated to Nuxt.

(The @nuxt/module-builder is advertised as "Compatible with Nuxt 3 and Nuxt Kit".)

Just checking, @nuxt/kit and @nuxt/schema do not depend on Vue 3 at all.

Maybe our understanding of "depends on" differs because those vue types are definitively not in Vue 2 (not even in 2.7):

https://github.com/nuxt/nuxt/blob/71225e50c5bc45c3feb7b189997659d880fe03f3/packages/schema/src/types/config.ts#L1-L1

@rchl
Copy link
Member

rchl commented Mar 9, 2023

I've missed before that https://github.com/nuxt/module-builder/blob/main/README.md says "Requirements: Nuxt 3 or Nuxt Bridge. Nuxt 2 is functional but not advised". So I guess it's not lying. The module will work but the types will be broken in Nuxt 2. This could perhaps be more clear.

@rchl
Copy link
Member

rchl commented Mar 9, 2023

And even if I can workaround it for the published package, the types (for example extends of NuxtConfig) will still be broken when locally developing this project because I can't avoid referencing @nuxt/schema (through @nuxt/kit) there.

@rchl
Copy link
Member

rchl commented Mar 12, 2023

I think the next step is to replace nuxt-module-builder with unbuild or plain rollup since I don't want to loose types while developing the module (even if it only affects test fixtures).

EDIT: Though that would also require dropping @nuxt/kit.

@rchl
Copy link
Member

rchl commented Mar 15, 2023

I don't have any outstanding issues with those changes so released as @nuxtjs/sentry@8.0.0-beta.10 (beta tag) and will probably merge this soon once I test it a little.

@rchl
Copy link
Member

rchl commented Mar 15, 2023

I don't consider this having any breaking changes (maybe only some minor in types but shouldn't affect normal usage) so I will aim to release it as a minor or patch bump.

@rchl rchl changed the title rewrite with module-builder and nuxt/kit refactor: rewrite with typescript and module-builder Mar 17, 2023
@rchl rchl merged commit 994f333 into nuxt-community:main Mar 17, 2023
@rchl rchl mentioned this pull request Mar 24, 2023
5 tasks
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.

7 participants