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

Introduce treemending work-around for pnpm #119

Merged
merged 1 commit into from
May 31, 2023
Merged

Conversation

kaisalmen
Copy link
Collaborator

@CGNonofr Patching directly doesn't work with pnpm. Every file of every used node_modules is contained in a data store. Patching files directly does not work. I debugged it locally, during the patch process suddenly files have already been patched and the patch process stops. My wild guess is that the file traversal is not consistent. 🤷‍♂️

Anyway, when a .pnpm directory is detected in monaco-editor path this workaround copies all esm files to a tmp dir patches everything and copies files back. This works in nicely in my test pnpm project.

@kaisalmen kaisalmen requested a review from CGNonofr May 31, 2023 12:05
@CGNonofr
Copy link
Contributor

I'm not familiar with pnpm, how does it help patching it elsewhere?

@kaisalmen
Copy link
Collaborator Author

kaisalmen commented May 31, 2023

It uses a datastore in your home directory, so you don't have any node_modules twice. The files in node_modules are linked/virtual. This makes problems during the patch process. Copying forth and back seems reliable. I came to this approach because pnpm patch creates a tmp directory where you apply changes and then the commit step moves it back.

It don't really like tool specific work arounds, but this is the one with least impact, IMO

@kaisalmen
Copy link
Collaborator Author

Btw, with this in place treemending as postinstall works with monaco-languageclient with npm, yarn and pnpm.

CGNonofr
CGNonofr previously approved these changes May 31, 2023
Copy link
Contributor

@CGNonofr CGNonofr left a comment

Choose a reason for hiding this comment

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

Feel like a hack to me, I hope it won't introduces other issues

maybe the patch approach is still not the best way...

Let's merge it and think about a better solution

src/monaco-treemending.ts Outdated Show resolved Hide resolved
@kaisalmen
Copy link
Collaborator Author

kaisalmen commented May 31, 2023

Feel like a hack to me, I hope it won't introduces other issues

Yes, I can't disagree 😎 I will test it later today with a new next release of monaco-languageclient.

@kaisalmen
Copy link
Collaborator Author

maybe the patch approach is still not the best way...

From the possibilities we have (monaco fork, polyfills) it is the best option.

@kaisalmen kaisalmen merged commit deb0e1c into main May 31, 2023
1 check passed
@kaisalmen kaisalmen deleted the pnpm-treemending-wa branch May 31, 2023 13:01
@CGNonofr
Copy link
Contributor

btw, can't https://pnpm.io/npmrc#public-hoist-pattern help us instead?

@kaisalmen
Copy link
Collaborator Author

Resolution is not the problem, but file traversal. I always ended up with files that were already patched although they were untouched before. It is really weird. It patched let's say 50 files and suddenly one was already patched. Ahhh, maybe it is contained twice and then the datastore stores it only once and then boom

@CGNonofr
Copy link
Contributor

CGNonofr commented May 31, 2023

reading about package-import-method, it seems that by default it uses a cloning mode, is this the issue? doesn't it fail when it's not supported? it says it doesn't work on mac

maybe switching to copy fix the issue (but it probably has a cost)

@kaisalmen
Copy link
Collaborator Author

maybe switching to copy fix the issue (but it probably has a cost)

then every pnpm user has to configure something.

That we have different tools doing the same, but not behaving consistent is the real issue, isn't it? 😉

@kaisalmen
Copy link
Collaborator Author

kaisalmen commented May 31, 2023

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants