Incorrect access modifier for SymbolConstructor.observable

[agubler]

@jason0x43 commented on Fri Jun 29 2018

Bug

SymbolConstrutor.observable has the default access modifier in @dojo/shim, but should be readonly. Currently it conflicts with recent node typings, which specifically flag observable as readonly.

Previous versions of the Node typings (< 10, 10.x before May) didn't include the observable property on SymbolConstructor, so fixing this shouldn't cause issues with code using older Node typings.

See also: theintern/intern#932

Package Version:

2.0.0

Code

Create a project that includes @dojo/shim@2.0.0 and @types/node@10.5.1 and try to build it.

Expected behavior:

It compiles.

Actual behavior:

node_modules/@theintern/leadfoot/node_modules/@dojo/shim/Symbol.d.ts:3:9 - error TS2687: All declarations of 'observable' must have identical modifiers.

3         observable: symbol;
          ~~~~~~~~~~


node_modules/@types/node/index.d.ts:169:14 - error TS2687: All declarations of 'observable' must have identical modifiers.

169     readonly observable: symbol;
                 ~~~~~~~~~~

[mgroenhoff]

I would like to see this one fixed. Should I create a PR for this?

[agubler]

Thanks @mgroenhoff, that would be great.

[mgroenhoff]

I'm having a bit of trouble fixing this because it is a little more complicated that just prefixing observable with readonly.

This is a subset if the dependency tree:

- @dojo/framework
    - @dojo/scripts
        - intern
            - @dojo/shim <-- ~0.2.7
            - @theintern/leadfoot
                - @dojo/shim <-- ~2.0.0

Using npm install, this results in the following tree:

- node_modules
    - @dojo
        - shim <-- v0.2.7
    - @theintern
        - leadfoot
            - node_modules
                - @dojo
                    - shim <-- v2.0.0

Types of both versions of @dojo/shim's are included within the compilation context and both versions include the following definition in their types:

declare global  {
    interface SymbolConstructor {
        observable: symbol;
    }
}

So if I fix src/shim/Symbol.ts and prefix it with readonly, I can not compile @dojo/framework anymore because I now have multiple different declarations of observable.

The proper way to fix this seems to first fix and release the earlier versions of @dojo/shim, update intern and @theintern/leadfoot use the new version and then fix @dojo/framework.

[agubler]

@mgroenhoff Thanks for the research, intern are in the process of removing their dojo dependencies (partially for these complex dependency reasons), so once that has been completed and released we should be able to fix framework.

[mgroenhoff]

I noticed @dojo/shim was made read-only so I guess the earlier versions can not even be fixed event if you wanted to.

@agubler That would indeed make things a lot easier!

[aciccarello]

For reference theintern/intern#936 is the issue to remove intern's domo dependency.

The symbol definition should still be updated to be readonly in preparation, correct?

framework/src/shim/Symbol.ts

Lines 6 to 8 in 07bdc06

	interface SymbolConstructor {
	observable: symbol;
	}

[mgroenhoff]

Correct. But that can only be done after intern and @theintern/leadfoot have their dojo dependencies removed and have been published otherwise you get compile errors.