search

ilearnio / module-alias

Register aliases of directories and custom module paths in Node
MIT License
1.76k stars 69 forks source link

Add default init behavior: look for package.json in process.cwd() #31

Closed Kehrlann closed 5 years ago

Kehrlann commented 6 years ago

We ran an issue with the default require('module-alias/register') because the node_modules in our prod environment was soft-linked to the current working directory.

Trying to load package.json from Node's current working directory does the trick, because we are running the app from a package manager ; so we added this as a possibility the init script will try out.

The no-argument init relies on the position of index.js in the filesystem, so it's tricky to test. And the default behavior currently has no test covering it - so we did not add a test.

Lines 147 and 148 seem redundant, you might want to have a look.

ilearnio commented 6 years ago

Checking process.cwd() seems reasonable though. I know there are no specs for this case but it is 100% must to have them before I can accept such PR.

Kehrlann commented 6 years ago

Thanks for all the feedback. Yes for now we initialize the module manually, we thought it'd be nice if we could auto-detect.

I've changed the behavior to use options.base if defined (even if wrong); otherwise try the parent dir/process.cwd().

I've added a spec for process.cwd(), it's simple but a bit weird as I do a process.chdir().

However I have not added a spec for default settings in the parent directory. This would require importing module alias from somewhere else than the source itself... I was thinking of dynamically copying index.js to test/src/node_modules before running a test, importing that copy, testing, then deleting it. What do you think ?

Kehrlann commented 6 years ago

I've taken your feedback into account and tried a few things. Some of them might be a bit bold.

  1. We test process.cwd() ; but we do not stub it - see comment : https://github.com/ilearnio/module-alias/pull/31#discussion_r218829826
  2. I've added a test for relative path import (__dirname/../..), but this requires some changes in spec.js, because it means we copy index.js around ; and import it multiple times. This might be a bit too messy, let me know what you think - I can revert this commit if you prefer.

Try commenting out either nodePath.join(__dirname, '../..') or process.cwd() in index.js, each should break one test only.

Cheers !

Kehrlann commented 6 years ago

Hi Nick,

Just a bump ... Have you had a chance to review the latest changes ? Sorry to ping you here, I'd just like to avoid having this PR too outdated :)

Cheers !

ilearnio commented 6 years ago

Hey, sorry for so big delay. Very busy recently at my job since we have deadlines and a lot of stuff to do, so really not much time left for other stuff. I'll try to get my hands around it on weekend, maybe earlier

Kehrlann commented 6 years ago

Thanks for answering :) No worries, take your time, doesn't have to be this week-end !

jlsan92 commented 5 years ago

Hello! Are you planning to merge this any time soon? I'm having the same problem when using this module in Docker 😞

Kehrlann commented 5 years ago

Hey @ilearnio,

It seems you haven't had much time to work on module-alias for a while. Would you be interested in some help ? It'd require to spend a bit of time to bring me up to speed on some things, maybe review a few PRs I do, but I'd love to contribute a bit more, triage issues, work on PRs, fix some outstanding issues (as much as I can, I am a bit busy as well).

Let me know !

Again, sorry to contact you here, couldn't find another way :(

Kehrlann commented 5 years ago

@jlsan92 - take a look at : https://github.com/ilearnio/module-alias/issues/43#issuecomment-444289416 ... The suggested fix might help.

If it doesn't, maybe open another issue with additional details ? e.g. your package.json, your Dockerfile, logs, etc.

ilearnio commented 5 years ago

Hey guys. Sorry for not being active in this project for a while, have crazy schedule at my work. Didn't have a normal weekend for long time.

@Kehrlann It would be really great if you can help to make it better, because I will be busy next couple of weeks for sure. I can add you as a collaborator. But please make sure that the specs are covering well every new case, especially if it's someone else's PRs. This package has more than million of installs as of now so don't want to ruin anybody's site, or my personal projects haha 😄

ilearnio commented 5 years ago

@Kehrlann wrote you on email