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

[🐞] qwikInsights vite plugin does not normalize paths like the qwikVite plugin #5337

Closed
intellix opened this issue Oct 20, 2023 · 3 comments · Fixed by #5366
Closed

[🐞] qwikInsights vite plugin does not normalize paths like the qwikVite plugin #5337

intellix opened this issue Oct 20, 2023 · 3 comments · Fixed by #5366
Labels
STATUS-1: needs triage New issue which needs to be triaged TYPE: bug Something isn't working

Comments

@intellix
Copy link
Contributor

Which component is affected?

Qwik Rollup / Vite plugin

Describe the bug

I'm playing with qwikInsights to improve performance of our Qwik app. We're also using qwik-nx to support multiple apps in the same codebase with shared libraries.

I noticed before that the dist directory was hardcoded in the original plugin and did a PR to fix it.

I now have 2 more problems:

  • The qwikVite plugin assumes q-insights.json exists within the /dist folder (doesn't take qwikInsights plugin options into account)
  • The qwikInsights plugin doesn't normalize paths like the qwikVite plugin does

So this means that if you configure the outDirs in qwikVite, the same value isn't going to work when passing in a value to the qwikInsights plugin:

qwikVite({
  client: {
    outDir: '../../dist/apps/myapp/client',
  },
  ssr: {
    outDir: '../../dist/apps/myapp/server',
  },
  tsconfigFileNames: ['tsconfig.app.json'],
}),
qwikInsights({
  outDir: '../../dist/apps/myapp/client',
  publicApiKey: loadEnv('', '.', '').PUBLIC_QWIK_INSIGHTS_KEY,
}),

In fact, even removing the ../../ from qwikInsights outdir is proving problematic for me as well.

Reproduction

N/A

Steps to reproduce

No response

System Info

N/A

Additional Information

No response

@intellix intellix added STATUS-1: needs triage New issue which needs to be triaged TYPE: bug Something isn't working labels Oct 20, 2023
@intellix
Copy link
Contributor Author

intellix commented Oct 20, 2023

I've looked at the codebase to try and do a PR to fix this myself, but I'm not sure what direction to take it cause it's a little complicated.

  • qwikVite.normalizePath depends on options passed into qwikVite({ ... }), and it's not a simple utility function so I'd need to pass in the same options to qwikInsights and create a plugin instance there to use the same normalization
  • qwikVite.loadQwikInsights assumes q-insights.json exists within dist, so the options passed into qwikInsights have no effect on where that's going to get loaded into.

I think it would be much easier for me to just move the functionality of the qwikInsights plugin into the qwikVite plugin as it's fairly simple anyway. We could pass a configuration for qwikInsights specifically and if the API_KEY exists, then it could download the bundle for consumption later:

qwikVite({
  client: {
    outDir: '../../dist/apps/myapp/client',
  },
  ssr: {
    outDir: '../../dist/apps/myapp/server',
  },
  insights: {
    outDir: '../../dist/apps/myapp/client',
    publicApiKey: loadEnv('', '.', '').PUBLIC_QWIK_INSIGHTS_KEY,
  },
  tsconfigFileNames: ['tsconfig.app.json'],
}),

Let me know if this is the direction we wanna take and if not, it would be great if someone could point me to an idea of how I can achieve it otherwise.

@mhevery
Copy link
Contributor

mhevery commented Oct 21, 2023

@gioboa perhaps you could offer your expertise here.

@gioboa
Copy link
Member

gioboa commented Oct 21, 2023

Yep. @intellix let's do pair programming

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
STATUS-1: needs triage New issue which needs to be triaged TYPE: bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants