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

refactor: isolate the googleapis-common module #1169

Merged
merged 2 commits into from May 15, 2018
Merged

refactor: isolate the googleapis-common module #1169

merged 2 commits into from May 15, 2018

Conversation

JustinBeckwith
Copy link
Contributor

This change entirely isolates the shared code required for all APIs from the rest of the module. It still includes these files in the overall build, but also introduces a lower build specifically for the new module. This is another step on the way to #806.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label May 14, 2018
branches:
ignore: /.*/
tags:
only: /^v[\d.]+$/
- publish_npm:
requires:
- node6

This comment was marked as spam.

command: cd src/shared
- run:
name: Install modules and dependencies.
command: npm install --unsafe-perm

This comment was marked as spam.

@@ -0,0 +1,38 @@
{
"name": "googleapis-common",
"version": "0.0.1",

This comment was marked as spam.

Copy link
Member

@alexander-fenster alexander-fenster left a comment

Choose a reason for hiding this comment

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

I'm mostly concerned about version 0.0.1. Other comments are optional.

@JustinBeckwith
Copy link
Contributor Author

Thanks @alexander-fenster - all fixed!

@alexander-fenster
Copy link
Member

Since we'll eventually have hundreds of npm modules published from this one repo, we'll need to rethink the current model of tagging and releasing based on the tag. I have no idea how exactly it should be done though (probably worth opening a separate issue to discuss future versioning).

@JustinBeckwith
Copy link
Contributor Author

One step at a time :)

Copy link
Contributor

@ofrobots ofrobots left a comment

Choose a reason for hiding this comment

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

Is the idea that googleapis-common would be published as a standalone package, and that this repo is now a monorepo? If so, let's use an established pattern of shared sub-module being in packages/googleapis-common subdirectory rather than ad-hoc src/shared. Have you considered spinning off to a separate repo though? If monorepos are the way to go, have you considered using https://github.com/lerna/lerna?

@ofrobots
Copy link
Contributor

Oh, and next time, can you please sequester autogenerated code changes into a separate commit. Reviewing changes is a lot harder when manual changes are intermixed with gobs of generated changes.

@JustinBeckwith
Copy link
Contributor Author

Great questions!

Is the idea that googleapis-common would be published as a standalone package, and that this repo is now a monorepo?

YES!

If so, let's use an established pattern of shared sub-module being in packages/googleapis-common subdirectory rather than ad-hoc src/shared.

I'm 100% on board with that. I am trying to make these changes incrementally. Today, the top level build/test/publish commands will continue to work, exactly as they used to. I'd like to propose landing this first, and working towards the properly named packages. Mostly, I'm worried about breaking the top level build.

If monorepos are the way to go, have you considered using https://github.com/lerna/lerna?

Yup! This is a point in time thing. Once I get all the sub-packages working independently, we will remove the top level build and use something like lerna to orchestrate the publish and linking.

Oh, and next time, can you please sequester autogenerated code changes into a separate commit. Reviewing changes is a lot harder when manual changes are intermixed with gobs of generated changes.

Sorry! Will do next time.

shame

@JustinBeckwith JustinBeckwith merged commit 2fef1ae into googleapis:master May 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants