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

fix: add missing dependency @vue/shared #520

Merged
merged 2 commits into from
Jun 2, 2024

Conversation

DrPhil
Copy link
Contributor

@DrPhil DrPhil commented Jan 30, 2023

In /src/add-routes-tracker.js there's an import from @vue/shared, which is not listed in the dependencies. This happens to work in many cases, since the dependency is used by many other vue-packages. But not reliably.

In our project we were unlucky enough to not have @vue/shared in the top-level node-modules after running npm prune --omit=dev, we only had it nested under other packages (since they didn't share the same version). This caused a MODULE_NOT_FOUND exception from node when we tried to load vue-gtag. Funnily enough it only happened in production - since the devs never pruned their packages.

I checked if there were any other missing dependencies, but could not find any. npx depcheck agrees that this is the only missing dependency.

Another possible solution is to remove the import and just define const isFunction = (val) => val is Function on your own.

@DrPhil
Copy link
Contributor Author

DrPhil commented Feb 17, 2023

@MatteoGabriele ping!

@MatteoGabriele
Copy link
Owner

That is really weird. I would never use a dependency just to check if that's a function or not. Must have been an automatic import from vscode that slipped out. I can't currently check this out, but I'd rather have a typeof === 'function' rather than including a dependency. Would u mind change that? Thanks for your pr

This partially reverts commit 8c2e395.
@DrPhil
Copy link
Contributor Author

DrPhil commented Feb 17, 2023

My bad. It looks like there are actually 3 usages of @vue/shared. The usages were introduced in #315 together with some ts fixes. I think the intention was good, since that's a package that everyone who uses this will have to install anyway - so we might as well dedupe the functions that already exist. Without explicitly marking it as a usage there might be cases where npm will not hoist the package to a place where we can reach it though.

I think marking it as a dependency might be best. But I've done as you suggested and semi-reverted the changes from that PR now, so now there are no usages of vue/shared again. :)

@DrPhil
Copy link
Contributor Author

DrPhil commented Mar 13, 2023

@MatteoGabriele ping again! :D

I think this might be ready for merging?

@MatteoGabriele
Copy link
Owner

MatteoGabriele commented Jun 2, 2024

@DrPhil Thanks for your work, and I'm really sorry for the delay. I've been trying to get back to this project lately.

@MatteoGabriele MatteoGabriele merged commit f3a1dac into MatteoGabriele:master Jun 2, 2024
5 checks passed
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.

None yet

2 participants