Templarian / MaterialDesign-React

Dist for Material Design Icons React Component for JS/TypeScript
https://materialdesignicons.com
MIT License
139 stars 20 forks source link

Move prop-types to dependencies #65

Closed nielsrowinbik closed 2 years ago

nielsrowinbik commented 2 years ago

Should fix #60, which forces users of this library to add prop-types to their own packages dependencies. Since prop-types is being used within this library, I don't think removing it like #60 suggests is really an option.

One thing to note is that npm did warn me about an old lockfile and has updated package-lock.json's format. I hope this won't be an issue. Here's the message it output:

npm WARN old lockfile The package-lock.json file was created with an old version of npm,
npm WARN old lockfile so supplemental metadata must be fetched from the registry.
npm WARN old lockfile 
npm WARN old lockfile This is a one-time fix-up, please be patient...

I have never seen this before so I'm not sure what the impact might be. I did run tests locally and they all pass.

Anticom commented 2 years ago

One thing to note is that npm did warn me about an old lockfile and has updated package-lock.json's format. [...]

npm v7 introduced a new lock file format, you can find more information about it here. Afaik this shouldn't be an issue as far as I'm concerned. All it's saying that it upgraded the lock file format in a backwards compatible way.

nielsrowinbik commented 2 years ago

Thanks @Anticom, I now remember upgrading npm right before making this PR. Can't believe I didn't link the two before. Anyways, all cleared up now.

@Templarian, does this PR look good to you?

Templarian commented 2 years ago

@nielsrowinbik @Anticom Let me work on getting a release out tomorrow with this fix.

(please @ me if I forget to push it out tomorrow, busy with vacation)

Templarian commented 2 years ago

@nielsrowinbik / @Anticom Just to be sure this PR change is required with latest node? Surprised more haven't reported it.

Anticom commented 2 years ago

@Templarian In my case the dependency layout is the following:

┌───────────────┐
│               │
│  Our project  │
│               │
└───────┬───────┘
        │
┌───────▼──────────────────────┐
│                              │
│  Reusable project package    │
│                              │
└───────┬──────────────────────┘
        │
┌───────▼──────────────────────────────────────┐
│                                              │
│  Our internal StoryBook component lib        │
│                                              │
└───────┬──────────────────────────────────────┘
        │
┌───────▼──────┐
│              │
│  @mdi/react  │
│              │
└──────────────┘

Where the "Reusable project pacakge" is living in a lerna mono repo. Now for some weird reason when running lerna bootstrap to wire together all the dependencies the transitive dev dependency of @mdi/react to prop-types is lost along the way which leads to build time errors. My suspicion is an issue with how lerna gets the job done under the hood - however I'm not 100% certain.

The whole point of this PR IMHO is less about a specific node / npm version and its requirements. But rather it makes sense to migrate the prop-types dependency from devDependencies to dependencies in any case, regardless of whether you find it beneficial to upgrade to a newer but backwards compatible lock file version.

To me the change is more about semantics, because prop-types is a run time dependency rather than a build time dependency.

nielsrowinbik commented 2 years ago

@Templarian Judging by the comments on #60 I don't think you remembered to create a release with this fix (both me and @Anticom also appear to have forgotten to remind you). Consider this your super late reminder 😄.

Templarian commented 2 years ago

Yes, publishing it in a bit today. I'm back to spending more time on the project.

Templarian commented 2 years ago

@nielsrowinbik Published at 1.6.0.

Templarian commented 2 years ago

Verified this all works great with React v18.0.0 also. Bumped the demo also to use React 18.0.0