State symbols use a fully qualified id instead of symbol?

[timotheeguerin]

The issue with having different version of cadl libraries installed is happening so many times

This is happening a lot if you are trying to work on multiple cadl project at once and the only way around is to create a workspace to link all the package version together. I also think this make it that if you have a dep on package A and package B which both depends on different version of library C then this would not work.

Wondering that if we have a createSymbol helper that will create a unique key for the library using the library name (e.g. @cadl-lang/rest.route)

Proposal

1. replace Symbol('foo') with Symbol.for("my-lib.foo")
2. create an helper createSymbol(or other name) that can get created in createLib similar to reportDiagnostic) that can autoamtically add the library name to make sure the symbol is unique.
3. Add warning if library is loaded multiple times at different versions

[bterlson]

Keys could also be created with Symbol.for('some-unique-identifier-for-the-library') instead of just Symbol()? I think @witemple-msft does this already.

[timotheeguerin]

oh didn't know this was a thing, yeah that would do the trick. one part that would still be useful is maybe the fact that it scope the key automatically. createSymbol is exported the same as reportDiagnostic

[nguerrera]

You could still get unexpected behavior if you put stuff into state with one version of a library and get it out with another.

Right now you fail to get anything out, but if you get out something with the wrong shape because the library changed the shape, it could still be hard to debug.

the only way around is to create a workspace to link all the package version together.

Is this mainly an issue in the language server / IDE?

I mentioned in another issue about compiler version mismatch that it is possible to create multiple language server instances. Maybe we should do that.

Or are there command line scenarios for a single project where this happens? I thought the peer dependencies were meant to resolve that?

[nguerrera]

On balance, using Symbol.for or equivalent is better than status quo since it will work more often than not when the version changes, but the state doesn't in the common case. So I'm not pushing back, but it's not very satisfying to me that you could still get silent bad behavior when a library changes too much and you load two versions.

[timotheeguerin]

@nguerrera it is also happening in the CLI, in this particular case we have this cadl-ranch repo that contains cadl scenarios that client generator will need to implemenent and cover. This repo has its own npm install of the dependency.
Now in autorest.python generator they want to call out to the cadl-ranch specs to generate but link their own generator. If they install the package as a published dependency then it works fine but there is times where you want to link a local install for developping both at the same time. In that case you get a version mismatch and you have to make either a global workspace and use pnpm to make the dependency unique or have the cadl-ranch repo as a submodule and have the autorest.python workspace include it. This feels very confusing everytime why nothing gets returned.

Maybe what we really need is the warning that if you load a library twice then it probably won't work but the workaround is still very hard to do and being able to just npm link another local package feels like something that should maybe work.

The other use case is: how the azure specs are going to be published in the azure-rest-api-specs repo vs consumed in the azure-sdk-for-$LANG repo and to make sure the dependency are unique. https://github.com/Azure/cadl-azure/issues/1763

[nguerrera]

Ha, I closed #785 against this.

I think it would be good if we did Symbol.for or equivalent and had a warning. We could maybe silence the warning if the version is the same but it got loaded twice from different physical locations. So it would work a lot of the time, but for that case where it behaves strangely, the warning would be a really helpful clue.

[timotheeguerin]

It is definitely tricky enabling this and adding the warning with the exception that the same version is loaded opens the question but what if its just a one of version/another compatible one. Maybe that needs to be explicitly specified when running cadl(I explicitly allow version 0.31.1 and 0.31.2 to work together)

[nguerrera]

I'm not confident in any scheme to declare which versions are compatible (either by convention like semver or some knob). My experience is that we always get it wrong and there's a breaking change where we don't expect one.

But it feels like if we don't allow exact version than some scenarios like the cadl-ranch have no way to get clean on the warning.

Or maybe it's the opposite, you silence the warning by declaring version ranges you're willing to consume? But that feels complicated. It seems like it can be annoying but should always be possible to line up the versions for production scenarios. And if you're just iterating with npm link, then you can just live with the warning?

[timotheeguerin]

Or maybe it's the opposite, you silence the warning by declaring version ranges you're willing to consume? But that feels complicated. It seems like it can be annoying but should always be possible to line up the versions for production scenarios. And if you're just iterating with npm link, then you can just live with the warning?

Yeah that's more what I was saying

[nguerrera]

Given that right now it's totally broken even for same version, seems like we could start by allowing that and merely warning for the rest. Then see what issues remain over time? Basically, don't go add knobs to specify compatible ranges until we get some mileage on the initial change and see we need to go farther.

Related: Can we relax the "compiler mismatch" error to warning on different version instead of error on different physical location if we do this. Or would it fall out as the same warning as this more general one?

My intuition is that it's a lot easier to go hunting for different versions, then to ensure that everything resolves to the same path. Would you agree?

[timotheeguerin]

Yeah compiler mistmatch also as long as we replace all our Symbol with Symbol.for in the compiler

[xirzec]

FWIW we went down this path before with tracing in Azure SDKs: https://github.com/Azure/azure-sdk-for-js/blob/47e53cc9f091bc33ccfc8f443b43fc40cb80a746/sdk/core/core-tracing/src/utils/cache.ts

It was a bit of a pain because we had to remember to bump the version in the symbol name whenever we made breaking changes... and then somehow remember which package versions corresponded to which symbol versions. It wasn't very easy to catch mistakes.

[markcowl]

PR is here: #871