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.

Sign up for GitHub

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

Jump to bottom

feat: Add graph of files for each discovered package #362

Merged
merged 6 commits into from Sep 27, 2022
Merged

feat: Add graph of files for each discovered package #362

merged 6 commits into from Sep 27, 2022

Conversation

wondersloth
Copy link
Contributor

@wondersloth wondersloth commented Sep 10, 2022 •
edited

  • This PR improves the migration-graph by adding a module (file) level migration graph to each discovered package.
  • This breaks out the base package logic from @rehearsal/migration-graph-ember into @rehearsal/migration-graph-shared.
  • Integrated with migrate command and test are passing ✅
  • Previous test fixtures were extracted out to it's own PR chore: create ember fixtures for testing against an app or addon #377

@wondersloth
Copy link
Contributor Author

wondersloth commented Sep 10, 2022 •
edited

It might be best to break down this PR into three.

  1. Add ember-app test fixtures.
  2. Add feature to migration-graph
  3. Integrate with migrate command in cli.

@wondersloth
Copy link
Contributor Author

I think I may need to rewrite some commits to have the commit message formatting that is being in this repo.

@wondersloth wondersloth changed the title FEAT migration graph with files feat: migration graph a graph of files for each discovered package Sep 10, 2022
@wondersloth wondersloth changed the title feat: migration graph a graph of files for each discovered package feat: Add graph of files for each discovered package Sep 10, 2022
lynchbomb
lynchbomb suggested changes Sep 12, 2022
View reviewed changes
Copy link
Contributor

@lynchbomb lynchbomb left a comment

Choose a reason for hiding this comment

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

We should be able to remove all the ember-3-28 fixture data and instead leverage scenario-tester.

Then we just point to a config which will build a given Ember app based on whichever config we pass.

scenario-tester doesn't have any doc's so look at this example:

Take a look at this example

If you search within the "babel-plugin-ember-test-metadata/blob/master/packages/test-scenarios" package you can see examples of how to implement scenario-tester.

For this PR maybe just split out the scenario-tester implementation from the extraction of dependency cruiser work

packages/cli/src/commands/migrate.ts Show resolved Hide resolved
packages/migration-graph-ember/package.json Outdated Show resolved Hide resolved
@wondersloth
Copy link
Contributor Author

This PR depends on #377 once rebase will be reduce two a couple commits.

@lynchbomb
Copy link
Contributor

This PR depends on #377 once rebase will be reduce two a couple commits.

@wondersloth #377 has been merged. if you could ping for re-review after rebase etc. ty!

Matthew Edwards added 3 commits September 26, 2022 10:46
Add module graph to package graph
8d99360 
- Create Package level migration graph with dependency-cruiser
- Decouple ember specific migration-graph from plain package
- Add MigrationStrategy, an easier to consume summary for migrating a package.
  - SouceFile object contains full and relative path.
- Add test coverage for migration-graph for `ember@3.28` app, addon, app with in-repo-addon
- Fixed issue where fastglob would not discover packages within an ember app because the parent
  directory matched an exclude pattern.
- Add options.entrypoint to MigrationGraph
- Fix bug with non-real paths for realtivePath creation.
Integrate with CLI
48c7ee1
Add TODOs for refactoring tests
7180bd8
lynchbomb
lynchbomb suggested changes Sep 26, 2022
View reviewed changes
packages/cli/src/commands/migrate.ts Show resolved Hide resolved
packages/cli/test/commands/migrate.test.ts Outdated Show resolved Hide resolved
@lynchbomb
Copy link
Contributor

a few minor comments then good to go

@wondersloth
Copy link
Contributor Author

@lynchbomb Addressed latest suggestions.

@lynchbomb lynchbomb merged commit 0150470 into master Sep 27, 2022
@lynchbomb lynchbomb deleted the FEAT-migration-graph-with-files branch September 27, 2022 13:54
lynchbomb pushed a commit that referenced this pull request Sep 27, 2022
@lynchbomb
feat: add graph of files for each discovered package (#362)
d15aa2a
lynchbomb pushed a commit that referenced this pull request Sep 27, 2022
@lynchbomb
feat: add graph of files for each discovered package (#362)
7af1c15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lynchbomb lynchbomb lynchbomb approved these changes

@haochuan haochuan Awaiting requested review from haochuan

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@wondersloth @lynchbomb