Skip to content
This repository has been archived by the owner on Apr 1, 2022. It is now read-only.

Yarn + npm merge #374

Merged
merged 38 commits into from
Oct 28, 2021
Merged

Yarn + npm merge #374

merged 38 commits into from
Oct 28, 2021

Conversation

skilly-lily
Copy link
Contributor

@skilly-lily skilly-lily commented Sep 21, 2021

Overview

Implementation status:

Build tool Workspaces No Workspaces
npm ✔️ ✔️
yarn version 1 ✔️ ✔️
yarn version 2 ✔️ ✔️

Acceptance criteria

  • All modules under Strategy.Node are well-laid-out.
  • Old modules are dead or moved
  • Tests pass
  • New tests are sensible and cover the new surface area
  • Any hacks to simplify testing/development are removed (e.g.: changing hlint config)
  • Review comment reminders are resolved
  • Docs are sufficient for hoisting into @liftM's work in [DNM] Overhaul documentation #368
  • Thorough testing and dependency analysis on several key repos: FOSSA core, yarn v1, berry (a.k.a. yarn v2)

Testing plan

Testing is divided into 2 categories:

  • Just helping out
    • Pick a repo and scan it, do spot checks on the results.
  • Deep dive quality check
    • Find a repo with a small-medium number of dependencies in package.json
    • Run the scan in --output mode
    • Check that all direct deps are marked as direct in the projects field of the output, and are present in sourceUnits.Imports.
    • Confirm that dev deps are not present in the graph at all, unless referred to (directly or transitively) by a direct dep.

Reviewer notes

Keep an eye out for my own reminder comments. This PR will not be merged until those are complete and confirmed by a reviewer, but they may stay during development to speedup customer interactions.

When reviewing, note the new modules, and review their API layouts:

  • Data.Glob - A strongly-typed wrapper around some basics of the System.Filepattern module, as well as some helpers needed to work with Path b t.
  • Data.Tagged - A strongly-typed wrapper around any value, such that the tag cannot be coerced silently. Use for when newtypes are not flexible enough, and has a functor instance so the the wrapped type may be modified without altering the tag.
  • Graphing.Hydrate - A generic hydrate, and a specialized hydrateDepsEnvs, which takes dep environments within a graph and recursively populates those environments to their children.
  • Test.Effect - A set of test utils and assertions which can be used alongside our effects. Most have identical names to their Test.Hspec counterparts.

Risks

If there are any special features in our JS support currently, they may not be implemented.

References

closes fossas/team-analysis#707

Checklist

  • I added tests for this PR's change (or confirmed tests are not viable).
  • If this PR introduced a user-visible change, I added documentation into docs/.
  • I updated Changelog.md if this change is externally facing. If this PR did not mark a release, I added my changes into an # Unreleased section at the top.
  • I linked this PR to any referenced GitHub issues, if they exist.

@skilly-lily skilly-lily added the DO NOT MERGE Not safe to merge into the mainline label Sep 21, 2021
.hlint.yaml Outdated Show resolved Hide resolved
src/App/Fossa/Analyze.hs Outdated Show resolved Hide resolved
src/App/Fossa/Analyze.hs Outdated Show resolved Hide resolved
src/Strategy/Node.hs Outdated Show resolved Hide resolved
src/Strategy/Node.hs Outdated Show resolved Hide resolved
src/Strategy/Node.hs Outdated Show resolved Hide resolved
@meghfossa meghfossa marked this pull request as ready for review September 22, 2021 00:58
@skilly-lily skilly-lily marked this pull request as draft September 22, 2021 03:24
@meghfossa
Copy link
Contributor

Ran into few issues with javascript directory in https://github.com/meghfossa/example-projects

@skilly-lily
Copy link
Contributor Author

@meghfossa I would only expect the with-yarn-v1 directory to work properly, since the other analyzers are not implemented yet.

@skilly-lily skilly-lily mentioned this pull request Sep 30, 2021
4 tasks
@skilly-lily skilly-lily force-pushed the yarn-npm-merge branch 2 times, most recently from 2d0835f to 6733920 Compare September 30, 2021 22:44
@skilly-lily skilly-lily marked this pull request as ready for review October 1, 2021 20:44
@skilly-lily skilly-lily requested review from meghfossa and cnr October 1, 2021 20:45
@skilly-lily skilly-lily changed the title [WIP] Yarn npm merge [DNM] Yarn npm merge Oct 1, 2021
@skilly-lily skilly-lily enabled auto-merge (squash) October 28, 2021 22:14
@skilly-lily skilly-lily merged commit 0570b34 into master Oct 28, 2021
@skilly-lily skilly-lily deleted the yarn-npm-merge branch October 28, 2021 22:30
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants