Skip to content

Commit

Permalink
fix: isConstruct is wrong when symlinking libraries (#955)
Browse files Browse the repository at this point in the history
`Construct.isConstruct` was still using (and recommending) `instanceof`,
even though `instanceof` can never be made to work reliably.

When we thought `instanceof` was safe to use again, it's because we
thought that `npm install` combined with `peerDependencies` would
make sure only one copy of `constructs` would ever be installed.

That's correct, but monorepos and users using `npm link` can
still mess up that guarantee, so we cannot rely on it after all.
  • Loading branch information
rix0rrr authored Mar 22, 2022
1 parent 8eee317 commit bef8e4d
Show file tree
Hide file tree
Showing 2 changed files with 40 additions and 4 deletions.
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,
});

0 comments on commit bef8e4d

Please sign in to comment.