Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # 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
@DamianGlowala DamianGlowala deleted the refactor/api-return-values branch August 17, 2023 18:54
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add ensurePackageInstalled utility
3 participants