-
Notifications
You must be signed in to change notification settings - Fork 595
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
Modularizing #91
Comments
We discussed this issue a while ago and ended up with bundling all clients into a single module.
We don't think it'll be an issue since clients are thin. A few more kilobytes won't hurt. We have plans to depend on Protobuf.js, it may be a game changer though.
That will make the release process unnecessarily complicated in our scale. Maintainers doesn't want to waste my time trying to version small tiny packages, and keep track of who should depend what and which version. |
I'm just here to help, so I won't fight too hard (:punch:!). However, the way you're suggesting goes against the Node/npm philosophy of small (the tinier the better!), maintainable, reusable modules. This is the ecosystem that is already in place, and one developers have come to expect when developing and consuming packages. Believe it or not, people do raise concern when a package grows to a larger kb size, but the big miss with our approach, that I don't think should be overlooked, is maintainability of this repo. When it grows to a size that all services are supported, this repo ends up with overlapping PRs and issues. It has been proven more effective to split out the "modules" within a large repo, allowing for maintainers with specialities in the individual services to continue offering support. Again, I'll follow the leader, but wouldn't mind hearing more thoughts considering these points :) |
@jgeewax @callmehiphop is this worth re-consideration? I've been splitting out pieces here and there (auth, resumable upload, gce-images, etc), and that's simply because if we kept all that stuff in this repo, nobody would ever want to contribute (or in the future for others, maintain). Now that we have quite a few services, with at least 4 more planned soon, maybe we should quit bloating up gcloud with unrelated code and modularize out? It would be much easier for someone to get started if they could just The gcloud package can live on as a wrapper around all of the modules. The one big show-stopper I can think of is, how would we integrate separate modules into our gcloud docs? We would have to be diligent about updating gcloud's package.json with any bumps from our modules. Are the cons worth the pros? Excited to hear your thoughts! |
I'm all for modularizing the library, we've discussed this before and as it stands today, we're probably breaking some node best practices by bundling several modules together. I think the obvious pros are:
As mentioned previously, a big con to this approach would be the added complexity to our release process. However, I believe there have been several discussions around making the upcoming bigtable api it's own module due to it's dependency on grpc, so the added complexity will probably end up being unavoidable anyways. |
If we use
That's a good point. (Relevant: googleapis/google-cloud-python#1094 (comment)): I was thinking the gcloud-common docs site could be the official "gcloud libraries docs" with different paths to show only one language's docs, for example: http://googlecloudplatform.github.io/gcloud-common/#/node === Node's API docs That would pull JSON files right from our repo, e.g.: gcloud-node/gh-pages/json/docs/(latestVersion).json We could format our external modules to go through the same docs -> json transform. We would iterate over the services gcloud supports, and show links for them in the nav bar. When an item is selected, it loads that version's json from the service's repo, e.g: gcloud-storage/gh-pages/json/docs/(version tagged from gcloud's package.json).json But a user who just wants to use our gcloud-storage module needs docs as well, so our same endpoint should be able to work as "The official documentation of the storage module": http://googlecloudplatform.github.io/gcloud-common/#/node/storage This might be crazy. |
I'm not a huge fan of having gcloud-common in the url. |
I think that might be our only option... that I can think of anyway. Each module would just redirect to the common URL. Copying the template over to every modules gh-pages would be pretty annoying. |
I'm definitely not saying we should copy over the template to every gh-pages, rather we could use hooks or submodules to inject the json into gcloud-node and then in the individual repositories just link to the appropriate api.. |
Also, something like this would have an impact on a common template for all the other apis since we'd need to offer version switching at the individual module level - which as of today no one else does. |
I'm trying to look out for the other gclouds. If we all share a template, it would be great to not have to individually host it. I haven't received feedback on that idea however. Please outline a plan that you see working and let's discuss that one :) |
Re: module versions, yep. We would have to define a bit of custom routing to handle it. Back to "are the cons worth the pros?" |
That's a good point, I think we're going to need @jgeewax to weigh in on that one. |
Forgive the long reply in advance. TL;DR: I don't think we should split things up into separate repos.... Just want to chime in here -- we definitely discussed this in a few other libraries. In gcloud-java for example, each service is it's own thing, but you can still add the "all gcloud" maven artifact and get it all -- however everything still lives in one git repository -- which I think is a very very good thing. The download size is not a thing I'm comfortable using as a reason to modularize. Internet speeds get faster and faster, and the typical experience is to npm install from your development machine, or your production machine (often in GCE for this library), both of which I'm comfortable saying "have a fast enough internet connection". Becoming aligned with Node best practices is a fine reason, however the benefit there is a bit thin. It also doesn't really solve a big problem for any class of customer (that I'm aware of). AFAIK, there are customers building big things (Snapchat, Spotify, etc) and there are tinkerers building small toys or side projects (or eventually turning into Snapchats). Both classes of people either want quick access to all cloud services (and don't care about download sizes) or they don't know what they want yet, but when they read about it somewhere, they're glad to not have to type N commands to install them and are happy that it all is "just there". Not needing to install extraneous libraries is a legitimate problem, however I'm not 100% certain that modularizing the library is the right way to fix that problem. In the CRC problem mentioned, I think there was an issue created somewhere basically saying "make that optional" -- not sure if that's possible. We're going to need to solve this issue for gRPC anyway, and since it can be used by almost every library (eventually at least), modularizing won't help here. Even further, reporting issues would become a bit confusing. One thing that people seem to really like is that when they have a problem with gcloud-node, they know where to file bugs (not a common thing at Google). If we split things apart, people now have ... many repositories to file an issue with, depending on the service they're using? What about auth for datastore... does that go in datastore ? or somewhere else? Finally, looking to our friends over at AWS and Azure, they ship everything in a single package, which I think is the right way to go (even if the only reason is "meeting customer expectations"). Most of our customers don't think "I am a Compute Engine customer and want the library for Compute Engine" or "I'm an EC2 customer and want the library for EC2", they think "I'm an AWS customer and want the library for AWS" or "I'm a Google Cloud customer and want the library for Google Cloud". Given all of these, I think we need to have some really good reasons if we're going to start splitting things up into chunks, and so far the 3 listed are not incorrect, but not super compelling either.... |
I had a nice micro-service, it has express and some other helper modules. It causes 83 modules to be installed. Fine. Then I needed to write a thing to BigQuery, nice, npm install gcloud and it pulls in... I think this repo could benefit by getting the number of dependencies down a bit. Either by modularization or a bit of re-use over the modules. Yeah, I have fast internet and all that but it is pretty nice to keep my docker images small and deployments fast. At least somewhat. |
Woah. 367 deps? That seems ... excessive. @stephenplusplus , when I do jjg@jjg1:~/projects/test/node_modules/gcloud/node_modules$ ls -l
total 108
drwxr-x--- 2 jjg eng 4096 Jan 21 16:46 arrify
drwxr-x--- 4 jjg eng 4096 Jan 21 16:46 async
drwxr-x--- 3 jjg eng 4096 Jan 21 16:46 concat-stream
drwxr-x--- 3 jjg eng 4096 Jan 21 16:46 create-error-class
drwxr-x--- 5 jjg eng 4096 Jan 21 16:46 dns-zonefile
drwxr-x--- 3 jjg eng 4096 Jan 21 16:46 duplexify
drwxr-x--- 2 jjg eng 4096 Jan 21 16:46 extend
drwxr-x--- 3 jjg eng 4096 Jan 21 16:46 gce-images
drwxr-x--- 3 jjg eng 4096 Jan 21 16:46 gcs-resumable-upload
drwxr-x--- 3 jjg eng 4096 Jan 21 16:46 google-auto-auth
drwxr-x--- 3 jjg eng 4096 Jan 21 16:46 hash-stream-validation
drwxr-x--- 3 jjg eng 4096 Jan 21 16:46 is
drwxr-x--- 5 jjg eng 4096 Jan 21 16:46 JSONStream
drwxr-x--- 2 jjg eng 4096 Jan 21 16:46 methmeth
drwxr-x--- 3 jjg eng 4096 Jan 21 16:46 mime-types
drwxr-x--- 6 jjg eng 4096 Jan 21 16:46 modelo
drwxr-x--- 3 jjg eng 4096 Jan 21 16:46 once
drwxr-x--- 2 jjg eng 4096 Jan 21 16:46 prop-assign
drwxr-x--- 2 jjg eng 4096 Jan 21 16:46 propprop
drwxr-x--- 12 jjg eng 4096 Jan 21 16:46 protobufjs
drwxr-x--- 3 jjg eng 4096 Jan 21 16:46 pumpify
drwxr-x--- 4 jjg eng 4096 Jan 21 16:46 request
drwxr-x--- 3 jjg eng 4096 Jan 21 16:46 retry-request
drwxr-x--- 3 jjg eng 4096 Jan 21 16:46 split-array-stream
drwxr-x--- 3 jjg eng 4096 Jan 21 16:46 stream-events
drwxr-x--- 2 jjg eng 4096 Jan 21 16:46 string-format-obj
drwxr-x--- 3 jjg eng 4096 Jan 21 16:46 through2 Do these really pull in 300+ modules in total ? |
Yep, producing and using many small modules is the npm way. npm version 3 deduplicates packages by default, so if a single module can satisfy multiple packages, it will only be fetched once. That's available in npm2 as well, but the user has to manually run |
Going to try and argue for modularization since it has sort of re-surfaced.
What about the number of dependencies? It's more or less in the same vein, modularizing would reduce the number of dependencies (and thus download size).
That's true, but assuming that we move future alpha APIs to their own package for pre-releases, this will happen regardless. We can try and steer users to report their issues on the main repo by putting it the individual projects READMEs and only linking the report issue buttons on the docs to the main repo. But we may end up having to think of a strategy to deal with this either way.
I don't think this would suggest we rid ourselves of the @stephenplusplus please chime in on this if you disagree, because I think you do! |
Mostly just taking a devil's advocate stance on this one, but this can actually work against us. If
I'm hoping we can avoid this (some reasons I gave here).
I think it's a pretty big weight to carry, and another point of failure in the release process. I'm mostly hung up on the following points. No doubt they all have solutions, it's a matter of how many "things" we want to have to go through, how they are maintained and understood. The fewer steps, the better. So please answer with as specific as possible solutions, because if it's simple enough, these points will cease to be issues.
|
My naive answer is to independently version each module with semver and we would mirror the version change in the main package. e.g. [email protected] A patch is released for gcloud-storage bumping it to 0.4.1. We then mirror the patch update to gcloud-node for 1.0.1. Again, this is naive, an alternative (and probably better) approach would be to mirror the plan gcloud-ruby intends on using as described here.
This is still being discussed, but I've proposed one possible solution. It might be worth mentioning that regardless of whether or not gcloud-node intends to modularize, other gcloud libraries will modularize, so this functionality will need to exist either which way.
Since we've elected to use a multi-file approach this would essentially mean that we would need to check out each individual module and run a script to generate the documentation. Our current release script does something similar to what I'm describing, ideally we would leverage that but instead of a single repository we would have to run on all the individual module repositories.
If we were to take this route, that would mean that gcloud-node would have fixed dependency versions. Because of this the "master" version would always be identical to the latest version (and so it becomes worthless). Therefor only when we increment the version of gcloud-node would docs be built. If and when the functionality for viewing individual modules comes into play we would then want to build certain sections of JSON when a version was released for that particular module.
Another unfortunate side effect of individual modules is issue reporting, we could point all issue submitting buttons to the main repository, document that when searching for issues to search against the main repository and add specific notes to the individual modules documentation that all issues should be reported to the main repository. That being said, there is absolutely no guarantee that anyone will post issues to the main repository, however, if we move to a system where we publish alpha APIs as a separate package, this issue is unavoidable regardless of whether or not we modularize all of the services. |
RE: versioning, as long as we use semver properly between all of our packages, it would be unnecessary to lock onto any specific version. Ranges should work: // gcloud-node package.json
{
name: 'gcloud',
version: '1.0.0',
dependencies: {
'gcloud-datastore': '^1.0.0', // We want 1.x.x, but not 2.x.x
'gcloud-storage': '^0.1.0' // We want 0.1.*, but not 0.2.0
}
} If either dependency releases a feature/fix, it is automatically picked up by default. If they release a breaking change, it won't be included until we manually release
Splitting services into multiple modules would require documentation to be available individually. You proposed the solution for that here: https://github.com/GoogleCloudPlatform/gcloud-common/issues/52 - which definitely solves the problem, though I do fear the weight the UX will put on users. I still can't visualize the solution for how a release of
We can always turn off issues on the module repos, and have the readme explain to open them at |
That's because I didn't explain it! My initial thought is to just manually release it.. but I'm sure if we really wanted to we could automate it via scripts + Travis |
FYI googleapis folks: @Anthm, @vsubramani@, @rok987 |
JJ asked me to investigate separate packages and write up my conclusions. Happy to have a discussion on https://github.com/GoogleCloudPlatform/gcloud-common/issues/138. |
We're modularized! |
- [ ] Regenerate this pull request now. PiperOrigin-RevId: 468790263 Source-Link: googleapis/googleapis@873ab45 Source-Link: googleapis/googleapis-gen@cb6f37a Copy-Tag: eyJwIjoiLmdpdGh1Yi8uT3dsQm90LnlhbWwiLCJoIjoiY2I2ZjM3YWVmZjJhMzQ3MmU0MGE3YmJhY2U4YzY3ZDc1ZTI0YmVlNSJ9
🤖 I have created a release *beep* *boop* --- ## [2.2.0](https://togithub.com/googleapis/nodejs-cloud-tpu/compare/v2.1.0...v2.2.0) (2022-11-10) ### Features * Add Secure Boot support to TPU v2alpha1 API ([#98](https://togithub.com/googleapis/nodejs-cloud-tpu/issues/98)) ([e4fc278](https://togithub.com/googleapis/nodejs-cloud-tpu/commit/e4fc27883278b8161bb7ad598dd83021e2467d99)) ### Bug Fixes * Allow passing gax instance to client constructor ([#96](https://togithub.com/googleapis/nodejs-cloud-tpu/issues/96)) ([d636ecf](https://togithub.com/googleapis/nodejs-cloud-tpu/commit/d636ecf4798258a71f289bd6a6add2cf45e6a2cb)) * Better support for fallback mode ([#91](https://togithub.com/googleapis/nodejs-cloud-tpu/issues/91)) ([a291abd](https://togithub.com/googleapis/nodejs-cloud-tpu/commit/a291abd4a0418eb375f9c4a27f19735afee4acca)) * Change import long to require ([#92](https://togithub.com/googleapis/nodejs-cloud-tpu/issues/92)) ([5de09bb](https://togithub.com/googleapis/nodejs-cloud-tpu/commit/5de09bb8786a790ff5a6d643f8493b6f6ea3c4ec)) * **deps:** Use google-gax v3.5.2 ([#104](https://togithub.com/googleapis/nodejs-cloud-tpu/issues/104)) ([86b8617](https://togithub.com/googleapis/nodejs-cloud-tpu/commit/86b86173fe7f8dd33e5cb6abb683f32d148670c6)) * Do not import the whole google-gax from proto JS ([#1553](https://togithub.com/googleapis/nodejs-cloud-tpu/issues/1553)) ([#95](https://togithub.com/googleapis/nodejs-cloud-tpu/issues/95)) ([e4289c1](https://togithub.com/googleapis/nodejs-cloud-tpu/commit/e4289c164ea2123947328ceddfb09cf083e50a19)) * Preserve default values in x-goog-request-params header ([#97](https://togithub.com/googleapis/nodejs-cloud-tpu/issues/97)) ([42310b7](https://togithub.com/googleapis/nodejs-cloud-tpu/commit/42310b7a99b51320e9924f8ecd1d75513b28b598)) * Regenerated protos JS and TS definitions ([#107](https://togithub.com/googleapis/nodejs-cloud-tpu/issues/107)) ([7e3cba5](https://togithub.com/googleapis/nodejs-cloud-tpu/commit/7e3cba5fd489373a7c460369d37f04c32e3cd9c3)) * Remove pip install statements ([#1546](https://togithub.com/googleapis/nodejs-cloud-tpu/issues/1546)) ([#94](https://togithub.com/googleapis/nodejs-cloud-tpu/issues/94)) ([7a26fe6](https://togithub.com/googleapis/nodejs-cloud-tpu/commit/7a26fe63b51c661bdd22bfafecc3d4291247dddf)) * use google-gax v3.3.0 ([e4289c1](https://togithub.com/googleapis/nodejs-cloud-tpu/commit/e4289c164ea2123947328ceddfb09cf083e50a19)) --- This PR was generated with [Release Please](https://togithub.com/googleapis/release-please). See [documentation](https://togithub.com/googleapis/release-please#release-please).
🤖 I have created a release *beep* *boop* --- ## [2.0.1](googleapis/nodejs-private-catalog@v2.0.0...v2.0.1) (2022-06-30) ### Bug Fixes * **docs:** describe fallback rest option ([#90](googleapis/nodejs-private-catalog#90)) ([d41ecfe](googleapis/nodejs-private-catalog@d41ecfe)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [jsdoc](https://togithub.com/jsdoc/jsdoc) | [`^3.6.6` -> `^4.0.0`](https://renovatebot.com/diffs/npm/jsdoc/3.6.11/4.0.0) | [![age](https://badges.renovateapi.com/packages/npm/jsdoc/4.0.0/age-slim)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://badges.renovateapi.com/packages/npm/jsdoc/4.0.0/adoption-slim)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://badges.renovateapi.com/packages/npm/jsdoc/4.0.0/compatibility-slim/3.6.11)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://badges.renovateapi.com/packages/npm/jsdoc/4.0.0/confidence-slim/3.6.11)](https://docs.renovatebot.com/merge-confidence/) | --- ### Release Notes <details> <summary>jsdoc/jsdoc</summary> ### [`v4.0.0`](https://togithub.com/jsdoc/jsdoc/blob/HEAD/CHANGES.md#​400-November-2022) [Compare Source](https://togithub.com/jsdoc/jsdoc/compare/3.6.11...084218523a7d69fec14a852ce680f374f526af28) - JSDoc releases now use [semantic versioning](https://semver.org/). If JSDoc makes backwards-incompatible changes in the future, the major version will be incremented. - JSDoc no longer uses the [`taffydb`](https://taffydb.com/) package. If your JSDoc template or plugin uses the `taffydb` package, see the [instructions for replacing `taffydb` with `@jsdoc/salty`](https://togithub.com/jsdoc/jsdoc/tree/main/packages/jsdoc-salty#use-salty-in-a-jsdoc-template). - JSDoc now supports Node.js 12.0.0 and later. </details> --- ### Configuration 📅 **Schedule**: Branch creation - "after 9am and before 3pm" (UTC), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR is behind base branch, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://app.renovatebot.com/dashboard#github/googleapis/nodejs-filestore). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC4xNy4xIiwidXBkYXRlZEluVmVyIjoiMzQuMTcuMSJ9-->
[![WhiteSource Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [@types/node](https://togithub.com/DefinitelyTyped/DefinitelyTyped) | [`^14.14.10` -> `^16.0.0`](https://renovatebot.com/diffs/npm/@types%2fnode/14.17.32/16.11.6) | [![age](https://badges.renovateapi.com/packages/npm/@types%2fnode/16.11.6/age-slim)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://badges.renovateapi.com/packages/npm/@types%2fnode/16.11.6/adoption-slim)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://badges.renovateapi.com/packages/npm/@types%2fnode/16.11.6/compatibility-slim/14.17.32)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://badges.renovateapi.com/packages/npm/@types%2fnode/16.11.6/confidence-slim/14.17.32)](https://docs.renovatebot.com/merge-confidence/) | --- ### Configuration 📅 **Schedule**: "after 9am and before 3pm" (UTC). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR is behind base branch, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox. --- This PR has been generated by [WhiteSource Renovate](https://renovate.whitesourcesoftware.com). View repository job log [here](https://app.renovatebot.com/dashboard#github/googleapis/nodejs-managed-identities).
🤖 I have created a release \*beep\* \*boop\* --- ### [1.2.1](https://www.github.com/googleapis/nodejs-retail/compare/v1.2.0...v1.2.1) (2021-08-17) ### Bug Fixes * **deps:** google-gax v2.24.1 ([#91](https://www.github.com/googleapis/nodejs-retail/issues/91)) ([893845a](https://www.github.com/googleapis/nodejs-retail/commit/893845aae9f43a41ad21f97000bc73da3fb985c0)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
By omitting default_version, we provide a hint to the generator not to generate src/index.ts
As we grow and support the suite of Cloud services, this repo could get very big, leading to:
I propose we split out into multiple repos, each representing a service (eg,
gcloud-datastore
,gcloud-storage
, etc.). If we have common functionality to share between services, we can expose that as a module as well.The text was updated successfully, but these errors were encountered: