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

Unregister default file: shortstop-handler #470

Open
jknight12882 opened this issue Aug 10, 2016 · 18 comments
Open

Unregister default file: shortstop-handler #470

jknight12882 opened this issue Aug 10, 2016 · 18 comments

Comments

@jknight12882
Copy link

jknight12882 commented Aug 10, 2016

This handler wreaks havoc when you have a local_module installed in package.json. It tries to execute a read in dependencies and crashes out with

[ 'error',
  'EISDIR: illegal operation on a directory, read',
  { stack: 'Error: EISDIR: illegal operation on a directory, read\n    at Error (native)' } ]
error: EISDIR: illegal operation on a directory, read stack=Error: EISDIR: illegal operation on a directory, read
    at Error (native), @timestamp=2016-08-10T02:35:48.205Z, logname=application, type=-application-, severity=ERROR, sequencenumber=1, logmessage=EISDIR: illegal operation on a directory, read
Wed, 10 Aug 2016 02:35:48 GMT uncaughtException EISDIR: illegal operation on a directory, read
Error: EISDIR: illegal operation on a directory, read
    at Error (native)
@tlivings
Copy link
Contributor

Hi there.

Could you provide some additional details about your configuration?

@jknight12882
Copy link
Author

jknight12882 commented Oct 12, 2016

Here is the relevant portion of our package.json

"dependencies": {
    ...
    "react": "file:./local_modules/react",
    "react-dom": "file:./local_modules/react-dom",
    ...
  }

When the app is launched via npm start, config variables are set on the environment as npm_config_* and then kraken tries to parse them. When it encounters the file: directive for the local module, it tries to parse and execute it and crashes out

@grawk
Copy link
Member

grawk commented Oct 12, 2016

the file handler uses fs.readFile. I'd expect it wouldn't work for what you're attempting since I assume "react" and "react-dom" are actually directories.

@jknight12882
Copy link
Author

jknight12882 commented Oct 12, 2016

@grawk that's exactly the point of this issue - this behavior is crashing my app. Since krakenjs by default registers the file: shortstop handler, and npm by default adds all the package.json config variables to the env, krakenjs is erroneously trying to parse the local modules part of the package.json and throws an error

@jknight12882
Copy link
Author

In other words, anybody using an npm supported file: dependency in their package.json is going to have a bad time

@grawk
Copy link
Member

grawk commented Oct 12, 2016

Oh, sorry. I didn't register that the file: directive was in your package.json as opposed to a config file...

@grawk
Copy link
Member

grawk commented Oct 12, 2016

There is currently no configurability around the set of shortstop handlers registered. That would seem to be the only resolution to this issue.

@tlivings
Copy link
Contributor

Thanks for bringing this to our attention. Another workaround may be to ignore values pulled in from sources other than configuration during resolution.

@jknight12882
Copy link
Author

Yeah, I was thinking the same thing - only parse things coming from the config.json for shortstop-handlers

@grawk
Copy link
Member

grawk commented Oct 12, 2016

Sounds like this issue should be filed against confit instead?

@tlivings
Copy link
Contributor

Package.json gets explicitly added here, not a confit concern.

@grawk
Copy link
Member

grawk commented Oct 13, 2016

I guess I need to reproduce this locally to fully understand what's happening... I'll give that a try tomorrow so I'm not talking out of my whatever.

@jknight12882
Copy link
Author

@tlivings Where is package.json being explicitly added? The issue I was encountering was because npm adds npm_config_* environment variables for config values in package.json.

@grawk
Copy link
Member

grawk commented Oct 13, 2016

OK. I have it reproduced locally. You just have to add a local module into the package.json... which should be clear enough from @jknight12882 's comments already. But for reading comprehension on my part.

@grawk
Copy link
Member

grawk commented Dec 23, 2016

Submitted a PR to confit: krakenjs/confit#65

This would give ability to configure a set of env properties for confit to ignore. Which in turn would prevent a shortstop-handler from attempting to process a string it shouldn't process.

The ignore property could then be set in kraken for the npm local_modules environment properties.

@grawk
Copy link
Member

grawk commented Dec 24, 2016

Made some changes to my fork of kraken-js to use the changes from the confit PR above:
grawk@f3a946f

This would merge the options passed into the kraken constructor under options.confit into the confit configurations used in kraken-js. So, if you used this:

var options = {
  confit: {
    envignore: ['npm_package_dependencies_react-dom', 'npm_package_dependencies_react']
  }
}
app.use(kraken(options));

Confit would simply ignore these environment variables and not attempt to match them to a shortstop handler.

@gshively11
Copy link
Contributor

Hello @grawk! I find myself running into this issue nearly 7 years later. It seems like a fix was found, but didn't make it all the way into the main kraken-js project. Will you accept a PR that implements the fix from your fork?

@grawk
Copy link
Member

grawk commented Jul 27, 2023

Hey @gshively11 .. happy to take a look and evaluate it. if it's an acceptable risk then we can move forward

gshively11 added a commit to gshively11/kraken-js that referenced this issue Jul 28, 2023
Options can now be passed directly to confit via `{ confit: {} }`.
This can be used to mitigate issue krakenjs#470.
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

No branches or pull requests

4 participants