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

Nuxt module improvements #9

Closed
pi0 opened this issue Jun 21, 2022 · 4 comments
Closed

Nuxt module improvements #9

pi0 opened this issue Jun 21, 2022 · 4 comments

Comments

@pi0
Copy link
Collaborator

pi0 commented Jun 21, 2022

Hi @harlan-zw and thanks for nice Nuxt module! Following up #374, I've made a quick review over the code and here are some improvement suggestions:

  • With hybrid rendering, both SSR and SPA modes can happen. Please avoid to depend on ssr option to conditionally inject plugin here. You can use runtime logic and serverRendered flag in the payloadto make a difference in the behavior
  • Since @vueuse/schema-org is an implicit dependency for Nuxt users (they install nuxt-schema-org), I would make an alias for @vueuse/schema-org to it's full path to avoid unwanted behavior with different package managers
@harlan-zw
Copy link
Collaborator

harlan-zw commented Jul 11, 2022

Hey @pi0

Appreciate your time in looking over the module, thanks! I've fixed up the first issue you mentioned, it makes a lot more sense to do it like this.

Would you mind elaborating on what you mean by the second point? I understand the issue but not too sure what you're recommending as the fix.

I tried this but it ended up throwing a lot of errors, seemingly the import breaks.

const { resolve } = createResolver(import.meta.url)
nuxt.options.alias['@vueuse/schema-org'] = resolve('@vueuse/schema-org')

@pi0
Copy link
Collaborator Author

pi0 commented Jul 11, 2022

The naming might be confusing but resolve is similar to path.resolve (await resolver.resolvePath is the one you are looking for)

harlan-zw added a commit that referenced this issue Jul 11, 2022
Resolves possible package manager issues, see #9
@harlan-zw
Copy link
Collaborator

harlan-zw commented Jul 11, 2022

Ah of course, thanks.

I added the alias, I had an error when it was resolving to the index.cjs file though:

[nuxt] [request error] Object.defineProperty called on non-object
  at Function.defineProperty (<anonymous>)  
  at $id_16230e0f (./.nuxt/dist/server/server.mjs:3011:8)  
  at __instantiateModule__ (./.nuxt/dist/server/server.mjs:6319:9)  
  at __ssrLoadModule__ (./.nuxt/dist/server/server.mjs:6257:25)  
  at ssrImport (./.nuxt/dist/server/server.mjs:6282:13)  
  at $id_a3506958 (./.nuxt/dist/server/server.mjs:2982:37)  
  at async __instantiateModule__ (./.nuxt/dist/server/server.mjs:6319:3)

So I changed it to resolve to the dist directory. Testing it seemed all good, but do you foresee any issues?

const schemaOrgPath = dirname(await resolvePath('@vueuse/schema-org'))
nuxt.options.alias['@vueuse/schema-org'] = schemaOrgPath

bca8fea#diff-be7ecc6bc17358942938e025d26bee5e5c3664db77e2be717b644d96d3222172R52

@harlan-zw
Copy link
Collaborator

Going to close this off as all the above issues were resolved, although have now been superseded by v1.

If you want to review the latest module, that would be awesome, but understand if you have limited capacity.

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

No branches or pull requests

2 participants