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

fix: isConstruct is wrong when symlinking libraries #955

Merged
merged 2 commits into from
Mar 22, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 15 additions & 1 deletion API.md
Original file line number Diff line number Diff line change
Expand Up @@ -80,10 +80,24 @@ toString(): string
__Returns__:
* <code>string</code>

#### *static* isConstruct(x)⚠️ <a id="constructs-construct-isconstruct"></a>
#### *static* isConstruct(x) <a id="constructs-construct-isconstruct"></a>

Checks if `x` is a construct.

Use this method instead of `instanceof` to properly detect `Construct`
instances, even when the construct library is symlinked.

Explanation: in JavaScript, multiple copies of the `constructs` library on
disk are seen as independent, completely different libraries. As a
consequence, the class `Construct` in each copy of the `constructs` library
is seen as a different class, and an instance of one class will not test as
`instanceof` the other class. `npm install` will not create installations
like this, but users may manually symlink construct libraries together or
use a monorepo tool: in those cases, multiple copies of the `constructs`
library can be accidentally installed, and `instanceof` will behave
unpredictably. It is safest to avoid using `instanceof`, and using
this type-testing method instead.

```ts
static isConstruct(x: any): boolean
```
Expand Down
28 changes: 25 additions & 3 deletions src/construct.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ import { MetadataEntry } from './metadata';
import { captureStackTrace } from './private/stack-trace';
import { addressOf } from './private/uniqueid';

const CONSTRUCT_SYM = Symbol.for('constructs.Construct');

/**
* Represents a construct.
*/
Expand Down Expand Up @@ -422,13 +424,26 @@ export class Node {
export class Construct implements IConstruct {
/**
* Checks if `x` is a construct.
*
* Use this method instead of `instanceof` to properly detect `Construct`
* instances, even when the construct library is symlinked.
*
* Explanation: in JavaScript, multiple copies of the `constructs` library on
* disk are seen as independent, completely different libraries. As a
* consequence, the class `Construct` in each copy of the `constructs` library
* is seen as a different class, and an instance of one class will not test as
* `instanceof` the other class. `npm install` will not create installations
* like this, but users may manually symlink construct libraries together or
* use a monorepo tool: in those cases, multiple copies of the `constructs`
* library can be accidentally installed, and `instanceof` will behave
* unpredictably. It is safest to avoid using `instanceof`, and using
* this type-testing method instead.
*
* @returns true if `x` is an object created from a class which extends `Construct`.
* @param x Any object
*
* @deprecated use `x instanceof Construct` instead
*/
public static isConstruct(x: any): x is Construct {
return x instanceof Construct;
return x && typeof x === 'object' && x[CONSTRUCT_SYM];
}

/**
Expand Down Expand Up @@ -536,3 +551,10 @@ export interface MetadataOptions {
*/
readonly traceFromFunction?: any;
}

// Mark all instances of 'Construct'
Object.defineProperty(Construct.prototype, CONSTRUCT_SYM, {
value: true,
enumerable: false,
writable: false,
});
Comment on lines +555 to +560
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not put this in the constructor? (is there any difference?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The difference is where the symbol lives: on the instance or on the class.

This puts it on the class, putting it in the construct (and presumably passing this instead of Construct.prototype) would put it on the instance.

Putting it on the class saves an instruction on every construct instantiation, and the memory of saving it in every symbol table (plus it might potentially have some impact on V8s optimizations... though I'm for sure not looking at any of that!)

It's probably a minor thing, but this ought to be slightly more efficient.