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

core: preserve prototype methods #844

Conversation

stephenplusplus
Copy link
Contributor

Before:

var gcloud = require('gcloud')({ projectId: 'grape-spaceship-123', keyFilename: 'key.json' });
var dataset = gcloud.datastore.dataset(); // throws "missing projectId" error

After:

var gcloud = require('gcloud')({ projectId: 'grape-spaceship-123', keyFilename: 'key.json' });
var dataset = gcloud.datastore.dataset(); // uses defaults from the gcloud instantiation

This used to work, so what happened? Well, I broke it.

When a user does:

var gcloud = require('gcloud')({ projectId: 'grape-spaceship-123', keyFilename: 'key.json' });

It prepares instances of the various modules (Storage, PubSub, etc) and then extends them so that static methods are picked up (Datastore.int, Storage.acl, etc). We have methods that are the same name as static methods, eg Datastore.dataset and Datastore.prototype.dataset so that if a user has an instance of a Datastore, they can access a properly scoped dataset. If they don't have an instance of a Datastore -- if they just did require('gcloud') without instantiating it with config options at that time -- Datastore.dataset will require configuration details, since you haven't given them anywhere else yet.

So, the fix in this PR preserves the prototype methods, not letting the static ones overwrite them.

// @callmehiphop -- need your help, buddy! Is this just madness?

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Sep 2, 2015
@callmehiphop
Copy link
Contributor

@callmehiphop
Copy link
Contributor

It does seem a little odd, but I think that's the cost of preserving the global configurations passed into gcloud. Function#bind doesn't preserve static members either, so I think this approach is really our only option.

Well, you could expose the configurations as a private member and then just return the constructor for each Class instead of a factory pattern. Then within each constructor, merge whatever local options were passed in with the global configurations set prior.

@stephenplusplus
Copy link
Contributor Author

Can you PR that? It sounds less weird!

@callmehiphop
Copy link
Contributor

Sure!

@callmehiphop
Copy link
Contributor

done - #845

@stephenplusplus
Copy link
Contributor Author

Nullified by #845

sofisl pushed a commit that referenced this pull request Jan 17, 2023
This PR was generated using Autosynth. 🌈

Synth log will be available here:
https://source.cloud.google.com/results/invocations/0d0efc4e-9ccd-472b-a303-4bbf020fc2e0/targets

- [ ] To automatically regenerate this PR, check this box.

Source-Link: googleapis/synthtool@ba9918c
sofisl pushed a commit that referenced this pull request Jan 24, 2023
sofisl pushed a commit that referenced this pull request Jan 25, 2023
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. core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants