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

Added ts path mapping between packages #395

Closed
wants to merge 1 commit into from

Conversation

trickstival
Copy link
Collaborator

@trickstival trickstival commented Sep 17, 2019

What does this PR contain?

It just changes the @posva packages path to be mapped to the src instead of the declaration files generated by the build in the dist folder.

Before that, when ctrl clicking the package @posva/vuefire-core inside another package files, it
redirected to the dist folder. Now it goes to the src.

I also removed one import that was pointing to a dist folder inside the core package (idk why it was doing that).

Motivation

It was really frustrating to look after the core files while debugging and end up in a declaration file. This should make the code flow easier to understand by ctrl clicking.

Edit:
I noticed that #384 should have its problem fixed too

Before that, when ctrl clicking the package @posva/vuefire-core inside another package files, it
redirected to the dist folder. Now it goes to the src.
@trickstival
Copy link
Collaborator Author

For some reason, it complains about files not being included when testing, but all tsconfig.json files are already extending the one in the project root, and this one already includes packages/@posva/*/src.

@codecov-io
Copy link

codecov-io commented Sep 17, 2019

Codecov Report

Merging #395 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #395   +/-   ##
=======================================
  Coverage   99.72%   99.72%           
=======================================
  Files          14       14           
  Lines         368      368           
  Branches       64       64           
=======================================
  Hits          367      367           
  Misses          1        1

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update db1e737...e48a73a. Read the comment docs.

Copy link
Member

@posva posva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't checked yet but we have to make sure that some of the transpilation still happens:
#329 #332

@trickstival
Copy link
Collaborator Author

I haven't checked yet but we have to make sure that some of the transpilation still happens:
#329 #332

@posva I have checked the target in all yarn build scripts across the project and it's es5. I'm not sure if that's the kind of check I have to do.

@posva
Copy link
Member

posva commented Sep 18, 2019

the best would be to test on IE and see if things are still working

@posva
Copy link
Member

posva commented Dec 31, 2019

I think that in the future Vuefire should just be in one single package. This would remove so many existing problems we have with tooling...

@trickstival
Copy link
Collaborator Author

I think that makes sense, but would vuexfire be distributed in the same package too?

@posva
Copy link
Member

posva commented Feb 7, 2020

yeah, everything in one tree-shakable package

@posva
Copy link
Member

posva commented Nov 18, 2022

Thank you for this and sorry I never camke back to you @trickstival
This ended up changing quite a lot and this PR isn't relevant anymore. The progress of the latest release can be followed at #1241

@posva posva closed this Nov 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants