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

Constructor does not normalize arguments in all cases #38

Closed
jmdobry opened this issue Jan 12, 2018 · 3 comments
Closed

Constructor does not normalize arguments in all cases #38

jmdobry opened this issue Jan 12, 2018 · 3 comments
Assignees
Labels
api: pubsub Issues related to the googleapis/nodejs-pubsub API. 🚨 This issue needs some love. triage me I really want to be triaged. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@jmdobry
Copy link
Contributor

jmdobry commented Jan 12, 2018

The PubSub constructor function contains

if (!(this instanceof PubSub)) {
  options = common.util.normalizeArguments(this, options);
  return new PubSub(options);
}

but that code is only activated when PubSub is used as a factory function. This is a problem when PubSub is used as a constructor, i.e. new PubSub(). common.util.normalizeArguments performs inference of the projectId, so when doing new PubSub(), it no longer recognizes your GCLOUD_PROJECT environment variable.

The problem of not calling normalizeArguments for all cases can be found in a bunch of Google Cloud Node libraries.

@jmdobry jmdobry added the type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. label Jan 12, 2018
@jmdobry
Copy link
Contributor Author

jmdobry commented Jan 12, 2018

Here's an example:

process.env.GCLOUD_PROJECT = 'my-project';
const PubSub = require('@google-cloud/pubsub');
PubSub().topic('my-topic').name; // projects/my-project/topics/my-topic
(new PubSub()).topic('my-topic').name; // projects/{{projectId}}/topics/my-topic

@stephenplusplus stephenplusplus self-assigned this Jan 12, 2018
@stephenplusplus
Copy link
Contributor

@callmehiphop it seems that there was a reason we didn't normalize the arguments outside of that block. Do you remember if there was one?

@callmehiphop
Copy link
Contributor

callmehiphop commented Jan 12, 2018

If I had a guess I would say it was because at one time we only had a single module in which the only accessors for services, like PubSub, were factory patterns. I think the ability to instantiate clients is more recent than this code and it simply got overlooked.

jmdobry added a commit that referenced this issue Jan 12, 2018
alexander-fenster pushed a commit that referenced this issue Jan 12, 2018
* Fix JSDoc and regenerate scaffolding.

* updating GAPIC generated files

* fix contributors, regenerate scaffolding

* run CI as non-root user to speed up grpc module install

* Remove temp fix for #38
stephenplusplus pushed a commit that referenced this issue Jun 11, 2018
## Version **0.16.0** of [google-proto-files](https://github.com/googleapis/nodejs-proto-files) was just published.

<table>
  <tr>
    <th align=left>
      Dependency
    </th>
    <td>
      <code>google-proto-files</code>
    </td>
  </tr>
  <tr>
      <th align=left>
       Current Version
      </th>
      <td>
        0.15.1
      </td>
    </tr>
  <tr>
    <th align=left>
      Type
    </th>
    <td>
      dependency
    </td>
  </tr>
</table>

The version **0.16.0** is **not covered** by your **current version range**.

If you don’t accept this pull request, your project will work just like it did before. However, you might be missing out on a bunch of new features, fixes and/or performance improvements from the dependency update.

It might be worth looking into these changes and trying to get this project onto the latest version of google-proto-files.

If you have a solid test suite and good coverage, a passing build is a strong indicator that you can take advantage of these changes directly by merging the proposed change into your project. If the build fails or you don’t have such unconditional trust in your tests, this branch is a great starting point for you to work on the update.


---


<details>
<summary>Release Notes</summary>
<strong>v0.16.0</strong>

<h2>Features</h2>
<ul>
<li>(<a class="issue-link js-issue-link" data-error-text="Failed to load issue title" data-id="316092585" data-permission-text="Issue title is private" data-url="googleapis/nodejs-proto-files#35" href="https://urls.greenkeeper.io/googleapis/nodejs-proto-files/pull/35">#35</a>): Allow passing <code>load()</code> options to protobuf.js. (Thanks, <a class="user-mention" data-hovercard-user-id="583593" data-octo-click="hovercard-link-click" data-octo-dimensions="link_type:self" href="https://urls.greenkeeper.io/nmccready">@nmccready</a>!)</li>
</ul>
</details>

<details>
<summary>Commits</summary>
<p>The new version differs by 19 commits.</p>
<ul>
<li><a href="https://urls.greenkeeper.io/googleapis/nodejs-proto-files/commit/b4d00bd0b7e6c2d4e0be1395739ded62bd9083e3"><code>b4d00bd</code></a> <code>0.16.0 (#44)</code></li>
<li><a href="https://urls.greenkeeper.io/googleapis/nodejs-proto-files/commit/47488981af49c1102737e60cad560f9609ac29d2"><code>4748898</code></a> <code>feat: allow load options (#35)</code></li>
<li><a href="https://urls.greenkeeper.io/googleapis/nodejs-proto-files/commit/91a34aa6ec2fb31932a03f4ed7756210146ea163"><code>91a34aa</code></a> <code>Merge pull request #42 from googleapis/greenkeeper/nyc-12.0.1</code></li>
<li><a href="https://urls.greenkeeper.io/googleapis/nodejs-proto-files/commit/fca6f7540c45b1ec3916602e1189cef289429908"><code>fca6f75</code></a> <code>Update package.json</code></li>
<li><a href="https://urls.greenkeeper.io/googleapis/nodejs-proto-files/commit/f0473ada67034a0a294d3f15dd1aa968acb5750a"><code>f0473ad</code></a> <code>chore(package): update nyc to version 12.0.1</code></li>
<li><a href="https://urls.greenkeeper.io/googleapis/nodejs-proto-files/commit/057563111be9bf9e97e02f893fbed08a9b465db2"><code>0575631</code></a> <code>chore: lock files maintenance (#41)</code></li>
<li><a href="https://urls.greenkeeper.io/googleapis/nodejs-proto-files/commit/5f38e7f77b6c14a5bbc256acb598fe10f4dbb693"><code>5f38e7f</code></a> <code>chore: lock files maintenance (#38)</code></li>
<li><a href="https://urls.greenkeeper.io/googleapis/nodejs-proto-files/commit/d077e0b86bf56135301a5240a15c4cbbabf2eb47"><code>d077e0b</code></a> <code>chore: test on node10 (#37)</code></li>
<li><a href="https://urls.greenkeeper.io/googleapis/nodejs-proto-files/commit/7855ccaca6a1f323719ef2c5a6c77ddd06953cb6"><code>7855cca</code></a> <code>chore: lock files maintenance (#36)</code></li>
<li><a href="https://urls.greenkeeper.io/googleapis/nodejs-proto-files/commit/e5548189863303d1973d54c5dc23d5eb599fbdaa"><code>e554818</code></a> <code>Merge pull request #32 from googleapis/greenkeeper/sinon-5.0.0</code></li>
<li><a href="https://urls.greenkeeper.io/googleapis/nodejs-proto-files/commit/0ecbb5e631d5918b8e1cf09e6ab1b6ed6d3baa64"><code>0ecbb5e</code></a> <code>Update package locks</code></li>
<li><a href="https://urls.greenkeeper.io/googleapis/nodejs-proto-files/commit/838e2d1d84db3e15a0a5a73242ac7399709d6a44"><code>838e2d1</code></a> <code>chore(package): update sinon to version 5.0.0</code></li>
<li><a href="https://urls.greenkeeper.io/googleapis/nodejs-proto-files/commit/fb18058f58c906ff9e90be80d4f312dbfb9ff68f"><code>fb18058</code></a> <code>chore: workaround for repo-tools EPERM (#33)</code></li>
<li><a href="https://urls.greenkeeper.io/googleapis/nodejs-proto-files/commit/cf5968e2974b0cecbb6a807ffde696ec37dd6fe6"><code>cf5968e</code></a> <code>chore: setup nighty build in CircleCI (#31)</code></li>
<li><a href="https://urls.greenkeeper.io/googleapis/nodejs-proto-files/commit/c4c973a41ee5810052fb0159ae2f83c18e6f866f"><code>c4c973a</code></a> <code>chore(package): update proxyquire to version 2.0.0 (#30)</code></li>
</ul>
<p>There are 19 commits in total.</p>
<p>See the <a href="https://urls.greenkeeper.io/googleapis/nodejs-proto-files/compare/89183d7d25f9c7e3b9097da820d9d84bd803a92f...b4d00bd0b7e6c2d4e0be1395739ded62bd9083e3">full diff</a></p>
</details>

<details>
  <summary>FAQ and help</summary>

  There is a collection of [frequently asked questions](https://greenkeeper.io/faq.html). If those don’t help, you can always [ask the humans behind Greenkeeper](https://github.com/greenkeeperio/greenkeeper/issues/new).
</details>

---


Your [Greenkeeper](https://greenkeeper.io) bot 🌴
stephenplusplus pushed a commit to stephenplusplus/nodejs-pubsub that referenced this issue Aug 31, 2018
@google-cloud-label-sync google-cloud-label-sync bot added the api: pubsub Issues related to the googleapis/nodejs-pubsub API. label Jan 31, 2020
@yoshi-automation yoshi-automation added 🚨 This issue needs some love. triage me I really want to be triaged. labels Apr 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: pubsub Issues related to the googleapis/nodejs-pubsub API. 🚨 This issue needs some love. triage me I really want to be triaged. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Development

No branches or pull requests

4 participants