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

dependenciesMeta:injected breaks hmr #4410

Open
farfromrefug opened this issue Mar 4, 2022 · 24 comments
Open

dependenciesMeta:injected breaks hmr #4410

farfromrefug opened this issue Mar 4, 2022 · 24 comments

Comments

@farfromrefug
Copy link

farfromrefug commented Mar 4, 2022

I am on macos.

Here is my setup:

  • module @nativescript-community/template-snippet with dependencies like this:
"dependencies":{
"@nativescript-community/ui-material-bottomnavigationbar": "file:../packages/bottomnavigationbar"
}, 
"dependenciesMeta": {
        "@nativescript-community/ui-material-bottomnavigationbar": {
            "injected": true
        }
}
  • application using @nativescript-community/template-snippet like this:
"dependencies": {
        "@nativescript-community/template-snippet": "file:../demo-snippets",
},
"dependenciesMeta": {
        "@nativescript-community/template-snippet": {
            "injected": true
        }
    }

The idea behind this is to have @nativescript-community/ui-material-bottomnavigationbar as a "direct" dependency in hoist mode in my app (which i need).

It works in the sense that i do see @nativescript-community/ui-material-bottomnavigationbar in my app "node_modules"
However HMR is broken for the file inside that package (node_modules/@nativescript-community/ui-material-bottomnavigationbar).
I have realized that the file modified at this path is changed but the HMR only picks it up once i "open" or access that file.
wondering if it comes from the way injected are "linked"?
Screenshot 2022-03-04 at 13 24 02

Also as shown in the screenshot the full absolute path is used for the injected module "path name". It is thus really long and could create issues like on Windows. Wouldn't it be best to use the relative path?

EDIT: if i use touch on the real path with all the ++ then HMR (webpack) also picks it up.

@farfromrefug
Copy link
Author

@zkochan was wondering if this 9c22c06 commit could help on this issue?
I am clearly seeing now that injected deps have their node_modules files not reflecting on changes from the orkiginal file. A touch call makes it work

@zkochan
Copy link
Member

zkochan commented Mar 19, 2022

Also as shown in the screenshot the full absolute path is used for the injected module "path name". It is thus really long and could create issues like on Windows. Wouldn't it be best to use the relative path?

This will be fixed in v7
faf830b

@zkochan
Copy link
Member

zkochan commented Mar 19, 2022

I guess we use copy-on-write on macos. So when you change the file, the injected file is not changed. Would a sync command help?

You may try to set package-import-method to hardlink

@farfromrefug
Copy link
Author

@zkochan thanks for fixing the path thing. As for the file not changed, tbh the sync command is not the solution. I mean you would expect this to work.
So you understand the reason i used dependenciesMeta:injected was because the dependencies from the "submodules" were not at the root level when using shamefully-hoist. It was solving this but creating the sync issue.

I think for me dependenciesMeta:injected is not the solution and i dont expect it (now) to be auto-sync. What i think is the real issue for me now is that shamefully-hoist does not work with file: protocol.
I think there is already an issue for this?

thanks for looking into this

@farfromrefug
Copy link
Author

@zkochan i have been testing 7 rc versions and the possiblity to drop dependenciesMeta:injected with file: dependencies.
I feel like something changed between rc2(i think) and. rc7.
Before hmr was working with file: deps without using dependenciesMeta:injected. With rc7 changes are not seen anymore. Any idea?

@farfromrefug
Copy link
Author

@zkochan sorry to bump this but this is a big issue which is still happening with 7.0.0. files in a file: dependency do not see changes (on macos). I should point out that i DONT use package-import-method to hardlink

@zkochan
Copy link
Member

zkochan commented May 4, 2022

In v7 the file protocol always works as injected. For the old behavior of file:, use link: instead.

@farfromrefug
Copy link
Author

@zkochan I can't use link because users might be using yarn or npm
anyway to enforce link not to be injected? may I ask why this happened?

@zkochan
Copy link
Member

zkochan commented May 4, 2022

This is the same way it works in Yarn.

@farfromrefug
Copy link
Author

@zkochan sorry not sure i understand your point. link: does not seem to work with npm so i cant use that in our OSS projects.
Now i am sorry to try and insist but as it is a breaking change from 6.x and that we cant seem to have an alternative with link: is there any way we can have an option for it to work with file: (while keeping all the changes you made in 7.x for file: like deep deps fixes )? Without it i am in a position where i cant use pnpm in our OSS projects anymore . All our repos here https://github.com/nativescript-community depends on this :s

@zkochan
Copy link
Member

zkochan commented May 4, 2022

Well, you said "I can't use link because users might be using yarn or npm"

but as I said, the file: protocol works the same way in both pnpm v7 and Yarn. The link: protocol also works the same way in pnpm and Yarn. So you really only care about npm compatibility, not npm and Yarn compatibility.

I don't get it why you allow your users to use random package managers in your repositories. You need to pick one package manager and commit its lockfile. With this "flexibility" you're just causing more work for yourself and nondeterminism.

An option to make it work as in npm might be added. Though I don't see too much value in it.

@farfromrefug
Copy link
Author

@zkochan we are an OSS project and we don't need nor want to force anyone to use one package manager or another. we all have our preferences. And npm (I would say sadly) is the default one for everyone on their computer.
Now I understand your point about behavior on yarn/pnpm. if there was an alternate solution for this without asking you to change something I would.
I think the main reason why I think an option would be lovely is that it is a breaking change from 6.x and thus might be breaking a lot of workspaces like ours.
And as always I thank you for taking the time to look at this and answer me.

@salvoravida
Copy link

I guess we use copy-on-write on macos. So when you change the file, the injected file is not changed. Would a sync command help?

You may try to set package-import-method to hardlink

Hi there, even if package-import-method=hardlink the hmr and /build directory is not sync. macOS 13.

file:../packA. link

@zkochan
Copy link
Member

zkochan commented Jun 22, 2022

It might be that the build overwrites the files in the package. In that case, only some "sync" command would help.

@farfromrefug
Copy link
Author

@zkochan the thing is that it was working with 6.x can't we get that back ? I mean using file: not using injected.
right now for me the file: seems broken. it seems to copy files and not link the folder.consequently I see no file change, addition, deletion.
I really hope we can fix this

@farfromrefug
Copy link
Author

@zkochan I am sorry to bump this but it is really a pain for complex workspace. I use file: in tens of projects and I keep on having to locally switch to link and revert before comitting.
I would really like for a solution for pnpm 7. Maybe we could have an option for file: to simply be exactly like link:? I think link: solves this issue and also #4623 (point being that link is not compatible with npm)

@zkochan
Copy link
Member

zkochan commented Jul 9, 2022

As I already said above, the file: protocol works the same in both pnpm and Yarn. It worked the same way in npm as well (until version 5).

I don't see enough value in this setting to make the code a lot more complex. I would rather change how the protocol works, which would be a breaking change. And not an easy change. I don't see demand for this change.

You are making everyone's life harder by choosing to support any package manager in your repositories. Just pick one and commit its lockfile. If it won't be pnpm, fine.

I use file: in tens of projects and I keep on having to locally switch to link and revert before committing.

A .pnpmfile.js might simplify your life. Replace the protocol in a hook:

module.exports = {
    hooks: {
        readPackage (pkg) {
            for (const [name, spec] of Object.entries(pkg.dependencies)) {
              pkg.dependencies[name] = spec.replace('file:', 'link:');
            }
            return pkg
        }
    }
}

It will only break on pnpm add <pkg> because the updated manifest will be written to the filesystem. But pnpm install will work as you want without changes to package.json.

@farfromrefug
Copy link
Author

@zkochan i kind of understand your point. though you can't say choose a package manager and make everyone s life easier. if I choose pnpm a lot of users will complain. I can say that for sure as we already tried.
now there is something where you loose me. why was it working just perfectly in pnpm 6.x ? all.my issues started with 7.x. Are you saying pnpm 6.x was not working correctly ?
now you say code complexity yes I understand if I ask you to make file work like link.
now knowing that and knowing 7.x broke some workspaces as it does not behaves like 6.x, this is why I am wondering if there could be a setting which would do what you propose to do with the sample script : when you encounter a file: treat it as a link:. it should require almost no code nor complexity and would solve all issues for ever.
I would simply need to add it to my workspace .pnpmrc .

I am trying to find an easy solution was which would.make everyone happy

@zkochan
Copy link
Member

zkochan commented Jul 9, 2022

if I choose pnpm a lot of users will complain.

I don't get it. Do contributors to the project complain? Contributors to your repositories?

why was it working just perfectly in pnpm 6.x ? all.my issues started with 7.x. Are you saying pnpm 6.x was not working correctly ?

In pnpm v6 file: and link: worked the same in pnpm. But many users wanted the Yarn behavior, so it was changed to Yarn's behavior in v7. TBH, the only reason it used a symlink in the past was because it was easier to implement like that.

@farfromrefug
Copy link
Author

@zkochan yes many contributors do not want to use pnpm and stick to npm. I asked explicitly if I could force pnpm and they asked me not to do it.
Having that in mine having an option to have file behave like link could create a bridge for many projects in the same situation as I.

Thanks for the explanation about 6.x vs 7.x it is much clearer now.

@farfromrefug
Copy link
Author

@zkochan i am looking at creating a PR for an option to treat file: as link:. I have been looking through the code a bit.
Do you think it would be enough to do it here ? it seems to be called to normalise dependencies.

@zkochan
Copy link
Member

zkochan commented Jul 19, 2022

Maybe. I don't know

@jereklas
Copy link
Contributor

jereklas commented Feb 9, 2023

Is there an update on this by any chance? I am hitting this case where I need to use the dependenciesMeta.*.injected feature, but when doing so get an awful developer experience due to the HMR breaking. I'm trying to find some type of work around but am not finding one yet.

UPDATE - I think I found a workaround for this issue with Vite by combining the dependenciesMeta.*.injected feature with Vite's server-watch functionality. So while I might have found something that works with Vite, I'm not sure about other build tools, and ideally a pnpm native solution can be figured out.

@octogonz
Copy link
Member

Update: We've proposed a pnpm-sync command that will include an API that tools can use to resync injected dependencies during watch mode: #4407 (comment)

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

No branches or pull requests

5 participants