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

Add support for vue single file components #124

Merged
merged 1 commit into from
Apr 15, 2024
Merged

Conversation

Havunen
Copy link
Contributor

@Havunen Havunen commented Aug 29, 2023

I realized vue sfc compiler v3 can also parse vue2 files which is enough to pull out dependencies from them, so I pinned the vue3 compiler as a dependency and that should fix the previous issue. I also merged latest changes to the PR

@Havunen
Copy link
Contributor Author

Havunen commented Aug 29, 2023

This pr relates to: pahen/madge#122 and dependents/node-filing-cabinet#111

package.json Outdated Show resolved Hide resolved
@Havunen Havunen requested a review from XhmikosR August 30, 2023 10:22
@Havunen
Copy link
Contributor Author

Havunen commented Sep 4, 2023

@XhmikosR can we merge these PRs please?

@Havunen
Copy link
Contributor Author

Havunen commented Oct 18, 2023

ping @XhmikosR

@Havunen
Copy link
Contributor Author

Havunen commented Nov 28, 2023

resolved conflicts again

@tangtangtangtangtang
Copy link

is there any chance to merge this PR, need vue support too
@Havunen @XhmikosR

@XhmikosR
Copy link
Member

XhmikosR commented Apr 3, 2024

Sorry, I personally won't merge a PR that adds 45 MB 49 MB to the deps. (https://packagephobia.com/result?p=detective-vue2)

@Havunen
Copy link
Contributor Author

Havunen commented Apr 4, 2024

It does not add that much, because it depends on the same deps as defined here to allow referencing them and npm will dedupe tthem.
See https://github.com/dependents/node-precinct/blob/main/package.json#L43-L56
all the code is here: https://github.com/Havunen/detective-vue2/blob/main/index.js
this is the only new dep: https://packagephobia.com/result?p=%40vue%2Fcompiler-sfc

@Havunen
Copy link
Contributor Author

Havunen commented Apr 4, 2024

One option is to not add a new package for detective-vue2 and implement this https://github.com/Havunen/detective-vue2/blob/main/index.js code directly here. I tried to follow the same coding pattern as used here

@XhmikosR
Copy link
Member

XhmikosR commented Apr 4, 2024

Node.js 14 is failing and I see + 25 packages (in this repo so both prod + dev).

Also, even if the size increase is 6.9 MB, it's not negligible, still.

@XhmikosR
Copy link
Member

@Havunen please cut a new detective-vue2 version with updated deps and rebase this. You are right that most deps should already be in our deps tree, so let's give this a go.

@Havunen
Copy link
Contributor Author

Havunen commented Apr 14, 2024

@XhmikosR I rebased the branch and also added 1 more test

@XhmikosR
Copy link
Member

@Havunen thanks. Can you please add tags to your repo and follow semver in the future?

I also submitted dependents/detective-vue2#1 which should be a major version bump probably, but feel free to adapt it to your needs.

Then we can land this and I'll cut a new minor release

@XhmikosR
Copy link
Member

Oh, also feel free to move the repo under the dependents org if you prefer.

@Havunen Havunen reopened this Apr 14, 2024
@Havunen
Copy link
Contributor Author

Havunen commented Apr 14, 2024

I re-applied the change for v2 and invited you to the repository as a collaborator. I cannot move it to dependents organization. I can make you admin there and you can then move it

@XhmikosR
Copy link
Member

Thanks! I don't see the repo settings tab, unfortunately :/

@XhmikosR XhmikosR changed the title added support for vue single file components Add support for vue single file components Apr 14, 2024
@Havunen
Copy link
Contributor Author

Havunen commented Apr 14, 2024

If you delete your fork I can move the ownership to you and then you can move it to the org

@XhmikosR
Copy link
Member

Alright, I deleted my fork. Weird that it's not possible to do it in another way. I also added you to the org, maybe you can move the repo now?

@Havunen
Copy link
Contributor Author

Havunen commented Apr 14, 2024

Okay, it seems to work now. I initialized the transfer

@XhmikosR XhmikosR merged commit 6d32f3c into dependents:main Apr 15, 2024
13 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.

3 participants