search

0x80 / isolate-package

Isolate a monorepo package with its internal dependencies to form a self-contained directory with a pruned lockfile
MIT License
121 stars 13 forks source link

Allow shared deps to reference other shared deps #14

Closed JoachimKoenigslieb closed 1 year ago

JoachimKoenigslieb commented 1 year ago

Allows a shared dependency to reference another shared dependency.

Because the whole point of this is to isolate a given package in a monorepo, we also have to do some dodging around the fact that this case is special and build these dependencies like they always was built.

0x80 commented 1 year ago

@JoachimKoenigslieb Thanks for looking into this!

As I mentioned here packages referencing other shared packages was already supported, with pnpm at least, and isolate-package is not specific to Firebase functions, so the semantics of the code would have change a bit, but that's just details.

Thanks for finding a solution already. I will look into it soon...

JoachimKoenigslieb commented 1 year ago

I'll try to setup some tests to compare NPM vs PNPM vs yarn with this behaviour. I'll see if I can get the integrated into the vitest setup already present.

Hopefully we can get it working and merged so I won't have to maintain a fork!

JoachimKoenigslieb commented 1 year ago

I've written a little test suite and it seems that:

The setup as-is passes correctly with npm and pnpm but fails with yarn. The fix i've written passes correctly with yarn and pnpm but wails with npm.

This is pretty unfortunate.

I've used the packageManager to switch the behaviour so that every package manager can install the isolated package manager successfully. This new update passes new tests!

If we wanted to be really careful, we could add a new config installedWith which could be yarn, npm or pnpm as there is no real guarantee that whatever that uses the isolate project will install using the same process as the whatever is used to build the isolate with. Tbh, I think this is good enough tough :)

0x80 commented 1 year ago

Thanks @JoachimKoenigslieb this looks good!

I think it's fine to ignore that edge case for now. It's not worth the time I think unless people start reporting issues that are related to it.

I realized that I don't yet enforce formatting, linting or coding standards. There are some things I do differently, like write all comments in blocks and start sentences with capital letter. Also I typically use object arguments once function parameters exceed 3. But these are things I can easily adjust myself after the PR is merged. I don't want to bother you with it now.

I will add a section about coding standards to the readme later.

Thanks for your help on this. I will try to test and merge it in the coming days.

JoachimKoenigslieb commented 1 year ago

Sounds good! I very much agree with the +3 args thing. I wish JS had named arguments...

If you need anything let me know!

0x80 commented 1 year ago

@JoachimKoenigslieb I started a new PR, because I wanted to take a different approach.

My finding are a little different from yours as well. I haven't been using your test suite, but I have been manually testing this with my project that uses nested dependencies.

Having a relative path for nested dependencies in my findings seemed to be required for both NPM and Yarn. Also, using the relative path was still compatible with PNPM, so I didn't see a need to make a distinction between package managers.

In the end I choose to make all file paths relative, since that also seems to be compatible.

I have already published a release candidate that you can try out with yarn add isolate-package@next -D

I'm still having trouble with NPM complaining about the lockfile, but that seems to be an issue that already existed #12

For Yarn and PNPM you can already start using the release candidate if you like.

I will also look at your tests later, because I haven't gotten around to it yet. I wonder why your test results were different then my findings when working directly with a real project, so I have to take a closer look and make sure they are both useful and correct.

But thank you for your help so far in working towards a solution. I appreciate it!

JoachimKoenigslieb commented 1 year ago

I've just updated to 1.3.1 and I think my deploys are working! Nice!

I think adding the tests would still be a worthwhile thing to do. It allows you to run automated tests against various package structures. Would also probably be pretty useful if you could then replicate failing scenarios with a tests later on.

If you are interested I can open up a focused PR that only adds the automated testing.