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

[GX-17918] Refactor resolve #64

Merged
merged 24 commits into from
Sep 6, 2019
Merged

[GX-17918] Refactor resolve #64

merged 24 commits into from
Sep 6, 2019

Conversation

kinetifex
Copy link
Contributor

@kinetifex kinetifex commented Sep 5, 2019

Summary

This is a refactor to @gasket/resolve necessary for consistent resolve patterns in cli, engine, presets, and metadata-plugin. This PR includes several fixes and changes.

I will highlight the improved separation of concerns changes next.

Changelog

Engine does not implement configured plugin loading

  • Problem: What plugins and how they were loaded was not available for uses outside the engine. For example metadata-plugin was accessing private member ._plugin of the engine, which was only a snapshot of useful info gathered by resolve.
  • Fix: Loading responsibility of @gasket/resolve which allows non-engine uses.

Engine does not initiate metadata

  • Problem: Updates to metadata require changes in two packages, plugin-engine and metadata-plugin. This results in a strange unnecessary relationship where the metadata-plugin would require a specific version of the engine.
  • Fix: All metadata definition responsibility of @gasket/metadata-plugin.

Presets do not resolve their own plugins

  • Problem: Because presets depend directly on @gasket/resolve, major changes to this package which would be used by the engine and cli would enter a broken state.
  • Fix: Now a responsibility of @gasket/resolve, used by the engine and cli.

Engine and metadata-plugin do not resolve package paths

  • Problem: Inconsistent plugin and package.json path resolves were occurring due to how resolve being done between these 3 packages, especially when using npm link or --preset-path with the cli.
  • Fix: Now solely a responsibility of @gasket/resolve

Test Plan

With the refactor, I was able to write more granular unit tests.

if (resolveFrom) {
this._resolveFrom = Array.isArray(resolveFrom) ? resolveFrom : [resolveFrom];
}
this._require = _require || require;
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 two most important features to preserve.

Copy link
Contributor

@indexzero indexzero left a comment

Choose a reason for hiding this comment

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

Reviewed the guts of @gasket/resolve (but not the upstream changes). The new separation of concerns feels a lot cleaner. I'm 👍 with a review from @DullReferenceException and/or another person with deep knowledge of @gasket/plugin-engine

@kinetifex kinetifex mentioned this pull request Sep 5, 2019
1 task
"test:coverage": "jest --coverage",
"posttest": "npm run lint"
"posttest": "npm run lint",
"docs": "jsdoc2md -t jsdoc2md/README.hbs lib/*.js > README.md; echo"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you need an empty echo here?

@kinetifex kinetifex merged commit fb51e0c into master Sep 6, 2019
@kinetifex kinetifex deleted the refactor-resolve branch September 6, 2019 15:59
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.

5 participants