search

electrode-io / electrode-native

A platform to ease integration&delivery of React Native apps in existing mobile applications
https://native.electrode.io
Other
726 stars 113 forks source link

Fixes bug with the latest version of the mkdirp #1894

Open fhkarczeski opened 1 year ago

fhkarczeski commented 1 year ago

Resolves the issue introduced by mkdirp v3.0.0 described here.

CLAassistant commented 1 year ago

CLA assistant check
All committers have signed the CLA.

fhkarczeski commented 1 year ago

@friederbluemle, @belemaire, @Karthiccc23, can you guys please take a look at this PR when you have a chance. It seems to be currently impacting all iOS container generation.

VinSpee commented 1 year ago

Hi! this is preventing my team from getting started with electrode-native. Any thoughts of fixing/releasing?

fhkarczeski commented 1 year ago

@VinSpee,

Our team is running the dev version with the fixes I proposed. You can check this document out on how to run it in dev: https://github.com/electrode-io/electrode-native/blob/master/CONTRIBUTING.md

You can manually add the changes from this PR to your dev version until someone from the Electrode team has time to check this PR out.

friederbluemle commented 1 year ago

Hi @fhkarczeski - Thanks a lot for the detailed analysis and the PR! I did a quick test locally, and was not able to reproduce - But I also noticed that version 3.0.1 of mkdirp was released just a couple of days ago. Is it possible that it fixed the issue you were seeing? I saw something in the changelog that default/named exports were removed, then added back again.

As a side note (not related to ERN): mkdirp is a widely used, low level package that is 13 years old - Introducing such a breaking change (major version or not), after all this time, for no good reason (other than "cleanup"?) seems pretty reckless.. But in any case, it is also not ideal that we omit the version number when adding the package, and it's definitely an area with some major potential for improvement/simplification.

fhkarczeski commented 1 year ago

Hello @friederbluemle!

I just ran some tests and you are correct, it seems that the latest version of mkdirp (3.0.1) fixes the issue we were having.

I agree with you that that this change was a little reckless on their part, and probably part of the reason why it was already addressed, but I also think it is possible to see other breaking changes in the future, since the version in our yarn.lock file is 3 major releases behind (^0.5.1).

Anyways, I haven't spent the time investigating how much impact mkdirp and invariant could cause. The one case I can think of is that if someone is using a feature in a miniapp specific to a particular version of these 2 packages (that is not the latest), then the iOS container gen can cause errors.

If you have ideas on how to improve and/or simplify this section of the code I would be interested in hearing!