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

Proposal fix for Issue #207 #215

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

Bloodb0ne
Copy link

Recently i have encountered the same issues as #207 . It looks like packages in the workspaces referencing another package in the same workspace dont have a shim create thats why readCmdShim fails, because it expects a shim file.
My solution just uses the src variable to create the new shim file after that if there occurs another reference to that package it will read the shim file and use that path.

In certain situations where you have internal workspace dependencies you dont have shims created so calling "readCmdShim" expects to read a shim file. Instead we just create the shim file using the file referenced in "src".
@lukebatchelor
Copy link
Member

Hiya. Thanks for the contribution.

None of the core maintainers do much of the way of windows development so finding and fixing these sorts of issues can be a pain for us, so we always appreciate any help in this area.

I'm not 100% sure why that function always assumes there is a cmd shim to be honest. @MarshallOfSound might have some sort of an idea. These functions were based on the implementations in npm and lerna if I'm not mistaken, so I'm a little bit cautious to change away from how they did it.

That being said, it definitely looks strange to me. I'm not sure on the best way to do this, but in the mean time, could you change the vars to lets and check the error code coming back from readCmdShim?

      if (err.code !== 'ENOTASHIM') {
        throw err;
      }

CC: @jamiebuilds as well, you'd probably have the most context as to why we always assume there's a cmd shim?

@MarshallOfSound
Copy link
Contributor

I'm hesitant to a change of default for this tbh, we've got a good thing going on Windows over in electron-forge. Can you provide a repro for whatever issues you are having @Bloodb0ne ? I'd also be infinitely more comfortable if the newly introduced code path was the fallback instead of the new primary. E.g.

var currentShimTarget;
try{
  currentShimTarget = path.resolve(
    path.dirname(src),
    await readCmdShim(src)
  );
} catch (err) {
  // Fallback to `src`
  currentShimTarget = src;
}

That way I know it won't break any of the existing functioning window stuff 😄

@ajaymathur
Copy link
Member

@MarshallOfSound Reproducible example repository shared in the issue:

https://github.com/kireerik/refo

from this comment

Rethrowing other errors than 'ENOTASHIM' and cleanup.
@Bloodb0ne
Copy link
Author

Bloodb0ne commented Jan 7, 2019

What happens in really is when you have reference a standard package you get a source with a directory that's your normal /node_modules/.bin but otherwise it has a dir for example /build/releases/bin/ ( the workspace is /build/releases ) where my .js file resides. My guess is that standard packages when they get installed the shim is auto created and that's not true for internal packages in a workspace.
I understand why you would be hesitant to merge when you use an existing implementation from npm to write that code.

@MarshallOfSound Here i created a minimal repository that shows what i described and has the same issue BoltIssueTest .

@Bloodb0ne
Copy link
Author

Seems like the shim creation is a big issue in a other repos too yarnpkg/yarn#6959

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.

4 participants