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

Fixes bug with the latest version of the mkdirp #1894

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

fhkarczeski
Copy link

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

…dirp library that contains breaking changes when generating a local iOS container.
@CLAassistant
Copy link

CLAassistant commented Apr 14, 2023

CLA assistant check
All committers have signed the CLA.

@fhkarczeski
Copy link
Author

@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
Copy link

VinSpee commented Apr 26, 2023

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

@fhkarczeski
Copy link
Author

@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
Copy link
Member

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
Copy link
Author

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!

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

Successfully merging this pull request may close these issues.

4 participants