Don't try to apply tree-mending patch twice

[CGNonofr]

No description provided.

[kaisalmen]

The monaco-editor version is wrong
The monaco-editor version is correct and the patch wasn't already applied
The monaco-editor version is correct and the patch was already applied

@CGNonofr what about joining the two approaches? I agree with the approach you outlined above, but a hash based verification can ensure it is really patched. Writing every single file hash to a file went to far, I think. What about just storing the hash sum of the result in monaco-editor-treemending.sha256 and then we can compare both instead of just storing a bool. I was just thinking my whole code is for the rubbish box otherwise. But it is your call! 🙂

[kaisalmen]

@CGNonofr FYI yarn doesn't like sole peerDependency definitions that's why I did this: https://github.com/TypeFox/monaco-languageclient/blob/main/packages/client/CHANGELOG.md#602---2023-05-24

[CGNonofr]

Are you suggesting replacing the peerDependency by a regular dependency?

Regarding the hash vs storing a boolean, the boolean approach seems simpler to me as it requires a lot less code, what would be the advantage of the hash? Also I can imagine reasons for the hash to be different (the os injecting hidden files ; mac loves to do it). Is adding a non standard property in package.json a problem for you? Microsoft already does it on monaco for the vscode and monaco commit hash

I know it's frustrating to put code into the trash but you have no idea how many times I did it here, I don't think it's a good reason to keep it

[kaisalmen]

Are you suggesting replacing the peerDependency by a regular dependency?

No, add the dependency back in addition as we did in the past. The peerDependency definition is still useful on flagging version violations (depending projects).

[kaisalmen]

what would be the advantage of the hash?

It really has a meaning, because it ensures the file content is identical. :-) The boolean value doesn't tell you anything: It could be tree-mended or not. Does Mac really mess with content of node_modules?

Whatever the solution is. We should think about a "global tree-mending-check-override" users can write in their own package.json like "treemending-check": "false" and your script tries to resolve this valus (default true). So, if someone has problems with the script this is the cure. With either of our solutions people may run into issues again, because there will be more package managers doing the same thing differently and non-uniform again. 😆

I know it's frustrating to put code into the trash but you have no idea how many times I did it here, I don't think it's a good reason to keep it

I know this game. 😉 Was kind of fun to realize such a thing with node.

[CGNonofr]

It really has a meaning, because it ensures the file content is identical. :-)

doesn't npm already garantee the content is identical if the version match? The only reason the content can be different is if something else touched it, which would make the hash check fail

The boolean value doesn't tell you anything: It could be tree-mended or not.

I'm not sure to understand that part, the whole point of the boolean is to know if it was tree-mended or not

Does Mac really mess with content of node_modules?

I'm not sure about it, maybe if you open it with the system browser?

With either of our solutions people may run into issues again, because there will be more package managers doing the same thing differently and non-uniform again

Maybe... that's why I put the patch at the root of the package so users can use it directly for edge-cases. the treemending script should work in the vast majority of users though

[kaisalmen]

doesn't npm already garantee the content is identical if the version match? The only reason the content can be different is if something else touched it, which would make the hash check fail

Just to prevent misunderstanding: With #115 the hash of the content of the complete esm folder is calculated during build/install of monaco-vscode-api and one hash value is now stored in monaco-editor-treemending.sha256 (I pushed an update some minutes ago). The proposed check during monaco-treemending does the same thing in the user's node_modules/monaco-editor/esm, So if the expectation stored in vscode/monaco-editor-treemending.sha256 matches the outcome tree-mending was already applied (fast-exit).

npm can't help here because it only knows the unmodified monaco-editor version. After tree-mending was applied monaco-editor (the files/driectory content) is in an unexpected state from npm's point of view, isn't it?

I'm not sure to understand that part, the whole point of the boolean is to know if it was tree-mended or not

It could be. It's just a hint

Maybe... that's why I put the patch at the root of the package so users can use it directly for edge-cases. the treemending script should work in the vast majority of users though

I added it to the postinstall of monaco-languageclient, that's the problem in TypeFox/monaco-languageclient#492), but I think this is the best approach if monaco-treemending exits without errors if everything was already patched. 👍

[kaisalmen]

@CGNonofr you decide what's the best solution. Important thing: script quits without complaint if everything is as expected! 👍

[CGNonofr]

Can you explain what you mean by

It could be. It's just a hint

?

[kaisalmen]

?

The boolean is stored in the package.json of monaco-editor. It tells it was patched, but someone could have just put it there. This approach is ok, but is not reliable enough. I thought of something similar in the beginning before I came up with the hashes. That's all.

[CGNonofr]

I'm not sure to understand why you think it's not reliable enough, if someone mess with the package.json, it's their fault if fails, don't you think? they can also alter any other files

[kaisalmen]

@CGNonofr go ahead with the boolean, we need to get this working.

[kaisalmen]

version comparison needs refinement.

[kaisalmen]

LGTM

[kaisalmen]

It's weird, your answer wasn't there yesterday Oo

I put this the answer outside the closed conversation by intention. Maybe GitHub had TGIF issues? 😉

What is the point of having both dependency and peerDependency ?

Yarn (1) as it does not resolve them and as we saw with monaco-languageclient (mlc), people still use it. The peerDependency will then serve as a warning if people define mismatching dependencies in their projects. with mlc I already define both peer and regular dependencies again.

Don't we need be this strict? The patch wwillonly work with this specific version.

Not with 0.37.1 / 1.78.0, but maybe in the future with 0.38.x / 1.79.0. We should use mlc to test it. I am actually thinking about creating dummy/test projects using yarn and pnpm to be able to test new versions of monaco-vscode-api (mva)/mlc.

[CGNonofr]

Btw, it the version is wrong, the treemending script will now fail