Make session private for Base class, add getter #690
Conversation
| class NewResource extends resource {} | ||
|
|
||
| NewResource.setClassProperties({ | ||
| CLIENT: RestClient, | ||
| CONFIG: config, | ||
| }); | ||
|
|
||
| Object.entries(NewResource.HAS_ONE).map(([_attribute, klass]) => { | ||
| (klass as typeof Base).setClassProperties({ | ||
| CLIENT: RestClient, | ||
| CONFIG: config, | ||
| }); | ||
| }); | ||
|
|
||
| Object.entries(NewResource.HAS_MANY).map(([_attribute, klass]) => { | ||
| (klass as typeof Base).setClassProperties({ | ||
| CLIENT: RestClient, | ||
| CONFIG: config, | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Updated to add configuration to each resource (and sub-resource) so that we can call the deprecated method inside the Base class (if needed).
There was a problem hiding this comment.
Any this also fixes any other requests that depend on a client, like
product.variants[1].save();which would fail due to the lack of a client before.
| "module": "commonjs", | ||
| "target": "es2015", |
There was a problem hiding this comment.
Needed for tsc to output CommonJS module format...
"module": "commonjs",
Needed for JavaScript private properties (e.g., #session)...
"target": "es2015",
There was a problem hiding this comment.
I don't necessarily know if this will affect any apps importing this package, but support for es2015 should be mostly there for node 14 (which is going EOL soon and is our latest supported version), so I don't expect the target will be a big problem.
There was a problem hiding this comment.
When "target" is "es5" (our previous setting), the .js output was commonjs.
Changing "target" to "es2015" resulted in ES Module output (es2015 or es6) ... explicitly changing "module" to be "commonjs" results in the output being "commonjs" again, resulting in virtually no impact to any apps updating to this version of the package.
| class NewResource extends resource {} | ||
|
|
||
| NewResource.setClassProperties({ | ||
| CLIENT: RestClient, | ||
| CONFIG: config, | ||
| }); | ||
|
|
||
| Object.entries(NewResource.HAS_ONE).map(([_attribute, klass]) => { | ||
| (klass as typeof Base).setClassProperties({ | ||
| CLIENT: RestClient, | ||
| CONFIG: config, | ||
| }); | ||
| }); | ||
|
|
||
| Object.entries(NewResource.HAS_MANY).map(([_attribute, klass]) => { | ||
| (klass as typeof Base).setClassProperties({ | ||
| CLIENT: RestClient, | ||
| CONFIG: config, | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Any this also fixes any other requests that depend on a client, like
product.variants[1].save();which would fail due to the lack of a client before.
| "module": "commonjs", | ||
| "target": "es2015", |
There was a problem hiding this comment.
I don't necessarily know if this will affect any apps importing this package, but support for es2015 should be mostly there for node 14 (which is going EOL soon and is our latest supported version), so I don't expect the target will be a big problem.
WHY are these changes introduced?
Fixes Shopify/first-party-library-planning#519
Fixes #685
WHAT is this pull request doing?
To prevent
sessionleaking via the REST resource objects, changethis.sessiontothis.#sessionand add a getter to allowthis.sessionto continue to work within the derived resource classes.Type of change
Checklist