-
Notifications
You must be signed in to change notification settings - Fork 316
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
Adopt monorepo - ie move openwpm-webext-instrumentation into this repo? #300
Comments
I still think this is a good idea to decouple the instrumentation from the OpenWPM crawling code. Ideally, we'd move the tests from this repo over to the instrumentation repo to decouple the development of the instrumentation even further. However, we can still refactor the tests to remove their dependency on the automation code and keep both in the same repo, if we decide to go monorepo.
@nhnt11 has felt a bunch of pain related to this. I'm curious to hear his thoughts.
The OpenWPM extension is basically a shell around the instrumentation library that has no use outside of OpenWPM's automation. So I wouldn't consider that when deciding whether to fold in the instrumentation.
+1 to publishing on npm, irrespective of our decision here.
Since I haven't experienced these development pain points yet myself (as I haven't needed to modify the extension), I'd like to hear from @nhnt11 first on his experience. |
The fact that you have felt the need to write mozilla/openwpm-webext-instrumentation#43 points to monorepo. |
Nihanth and I spoke about this yesterday, and we're both leaning towards consolidating the repos for the time being. Our thinking is that we still need to iterate on the tests and instrumentation, and that will be much easier to develop and review if everything is in one place. Once we have a fairly stable library with self-contained tests, we can revisit moving these back our to a separate repo. |
This is a bit convoluted, and I find myself easily getting onto a "train of overthought". It sounds complicated to merge the repos now and split them up again later, but I think that development in the immediate future will be positively impacted by a mono-repo approach. The biggest problem, I think, is that the tests are in one repo and the code is in another. We can keep things modular and split it up properly if needed when things are more mature. |
On our meeting last week we concluded the follow long term order of events.
This allows us to move faster towards a stable updated framework using monorepo for the foreseeable future, while maintaining the long term goal of letting the instrumentation be an independent repository and Github project. |
TODO for monorepo:
After the above, we can publish to npm (#329) and add instrumentation-specific tests (#330) |
Progress update:
|
Opening this for discussion.
I am finding various friction points when attempting to customize OpenWPM that relates to openwpm-webext-instrumentation being in a separate repository.
The main advantage of having openwpm-webext-instrumentation (the general data gathering bits that can be used in any web extension) in a separate repo is to be able to let it's development and consumption be decoupled from the OpenWPM crawl orchestration logic.
However, the mix between monorepo and multirepo currently seems to cause a net loss in development progress, with issues and PRs being split across repos, especially for modifications that require changes in both codebases to make sense.
Also, with the decision of moving the new OpenWPM web extension from a separate repo into the main OpenWPM repo, it seems we tend towards monorepo structure also for the new webext-based OpenWPM branch.
Another (smaller) advantage of having openwpm-webext-instrumentation as a separate repository is the ability to consume it directly from github using npm without having to have an npm package published, but this can be alleviated by simply publishing an npm package.
If @englehardt and @nhnt11 are also pro going monorepo here, I can setup the PR to make that happen (landing it in
OpenWPM/automation/Extension/webext-instrumentation
). I will also publish the appropriate npm packages.The text was updated successfully, but these errors were encountered: