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

refactor!: improve library #75

Merged
merged 15 commits into from
Aug 16, 2023
Merged

Conversation

DamianGlowala
Copy link
Contributor

@DamianGlowala DamianGlowala commented Jun 29, 2023

This PR:

  • makes API functions' return values consistent (avoids suppressing errors thrown by execa or returning {})
  • cleans up the test/ folder
  • addresses workspace arg handling based on the package manager with corresponding tests
  • ensures error gets thrown when package manager auto-detection fails to find any
  • organises util functions into separate files in the utils/ directory
  • marks addDevDependency as deprecated (either we should provide equivalent removeDevDependency or might be better to delete it entirely) - open to opinions, but knowing this is as simple as addDependency(name, { dev: true }) makes me think if we should get rid of it in a major release
  • adds includeParentDirs option to detectPackageManager util function, which prevents from bubbling the search up if not set to true explicitly
  • resolves Add ensurePackageInstalled utility #53

@pi0, are segmentation faults when using corepack with npm on ubuntu worth reporting? Will revert to the initial workaround for now anyway.

@codecov
Copy link

codecov bot commented Jun 29, 2023

Codecov Report

Merging #75 (b953c10) into main (0e22f7f) will increase coverage by 1.48%.
The diff coverage is 98.85%.

@@            Coverage Diff             @@
##             main      #75      +/-   ##
==========================================
+ Coverage   96.53%   98.02%   +1.48%     
==========================================
  Files           5        5              
  Lines         289      405     +116     
  Branches       34       64      +30     
==========================================
+ Hits          279      397     +118     
+ Misses         10        8       -2     
Files Changed Coverage Δ
src/api.ts 95.94% <98.05%> (+1.05%) ⬆️
src/_utils.ts 98.42% <98.42%> (ø)
src/index.ts 100.00% <100.00%> (ø)
src/package-manager.ts 100.00% <100.00%> (ø)
src/types.ts 100.00% <100.00%> (ø)

@DamianGlowala DamianGlowala changed the title feat: improve workspace arg handling and API return values [WIP] feat: improve workspace arg handling and API return values Jul 6, 2023
@DamianGlowala DamianGlowala changed the title [WIP] feat: improve workspace arg handling and API return values feat: library overhaul Jul 6, 2023
@AndreyYolkin
Copy link

LGTM

@pi0 pi0 changed the title feat: library overhaul refactor: library overhaul Aug 16, 2023
@pi0 pi0 changed the title refactor: library overhaul refactor!: improve library Aug 16, 2023
@pi0
Copy link
Member

pi0 commented Aug 16, 2023

Thanks so much for the time put into this PR dear @DamianGlowala ❤️

I have put a few refactors to keep the repo simplified and this mergeable.

There are really nice changes and i don't want to miss them but i couldn't do it without making a breaking change release and we lack proper changelog... therefore i ask you to please next times, consider smaller mergable PRs 🙏🏼

@pi0 pi0 merged commit bee69fd into unjs:main Aug 16, 2023
5 checks passed
@DamianGlowala DamianGlowala deleted the refactor/api-return-values branch August 17, 2023 18:54
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.

Add ensurePackageInstalled utility
3 participants