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

WIP ref(worker): switch to @azure/brigadier #585

Merged

Conversation

technosophos
Copy link
Contributor

This refactors the Brigade internal library to use @Azure/brigadier. This contains breaking changes in the pathing. (See https://www.npmjs.com/package/@azure/brigadier)

This also intercepts and overrides require("@azure/brigadier") for brigade.js files.

@technosophos
Copy link
Contributor Author

technosophos commented Aug 8, 2018

Still todo:

  • Manual testing
  • Publish Azure/brigadier GitHub project

@kooba kooba mentioned this pull request Nov 5, 2018
This refactors the Brigade internal library to use @Azure/brigadier. This contains breaking changes in the pathing.

This also intercepts and overrides `require("@azure/brigadier")` for brigade.js files.
@technosophos technosophos force-pushed the feat/refactor-npm-brigadier branch 2 times, most recently from ae21bc7 to 1449798 Compare January 4, 2019 01:02
@@ -1,11 +1,11 @@
/**
* Module app is the main application runner.
*/
import * as events from "./events";
import * as events from "@azure/brigadier/out/events";
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I am mildly annoyed by the fact that /out/ should be included every time the package is used - I suspect there's a configuration property that can help us avoid this - I'll look into it tomorrow morning.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, mildly was an understatement.

Copy link
Contributor

Choose a reason for hiding this comment

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

Update: found a solution for pretty import paths - not ideal, but will PR against Brigadier.


constructor(job: jobs.Job, e: BrigadeEvent, project: Project) {
public init<T extends jobs.Job>(job: T, e: BrigadeEvent, project: Project) {
// init takes a generic so we can run this against mocks as well as against the real
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: can the function comment be before the function definition?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Copy link
Contributor

@radu-matei radu-matei left a comment

Choose a reason for hiding this comment

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

LGTM

(With a minor nit about the comment before the function, and the fact that import paths for Brigadier need to include /out).

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.

None yet

2 participants