bolt icon indicating copy to clipboard operation
bolt copied to clipboard

Proposal fix for Issue #207

Open Bloodb0ne opened this issue 6 years ago • 5 comments
trafficstars

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.

Bloodb0ne avatar Jan 06 '19 18:01 Bloodb0ne

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?

lukebatchelor avatar Jan 07 '19 03:01 lukebatchelor

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 😄

MarshallOfSound avatar Jan 07 '19 05:01 MarshallOfSound

@MarshallOfSound Reproducible example repository shared in the issue:

https://github.com/kireerik/refo

from this comment

ajaymathur avatar Jan 07 '19 05:01 ajaymathur

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 avatar Jan 07 '19 10:01 Bloodb0ne

Seems like the shim creation is a big issue in a other repos too https://github.com/yarnpkg/yarn/pull/6959

Bloodb0ne avatar Jan 29 '19 22:01 Bloodb0ne