Skip to content

LG-11001: Mark i18n as peerDependency in certain @18f/identity- packages#9257

Merged
allthesignals merged 2 commits intomainfrom
wmg/11001-dedupe-i18n
Sep 25, 2023
Merged

LG-11001: Mark i18n as peerDependency in certain @18f/identity- packages#9257
allthesignals merged 2 commits intomainfrom
wmg/11001-dedupe-i18n

Conversation

@allthesignals
Copy link
Contributor

🎫 Ticket

🛠 Summary of changes

It makes i18n a peer dependency and prevents duplicate copies by configuring Webpack to exclude it during the build.

I marked the affected packages as a major version bump because it would be breaking.

📜 Testing Plan

I've spot-checked this in the individual builds and identity-site, and found only one copy.

I sanity-checked this by seeing how many copies were included in the build output before these changes. There were 2 extra copies!

@allthesignals allthesignals requested review from a team and racingspider September 25, 2023 13:50
conditionNames: ['source'],
},
externals: /^(?!(@18f\/identity-|\.))/,
externals: [/^(?!(@18f\/identity-|\.))/, '@18f/identity-i18n'],
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really love the way this is set up... we would need to get Webpack to distinguish between packages that are available via workspaces and packages that are published on the npm registry. Otherwise, we are left explicitly specifying this....

Copy link
Contributor

@gina-yamada gina-yamada left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

App working as expected. A 'requires' list added to the components README would be nice, but non-blocking. Good idea bumping up to major version b/c of breaking change. LGTM- approving

@aduth
Copy link
Contributor

aduth commented Sep 25, 2023

I wonder -- could it work to just mark the package as an external and list it as a dependency rather than a peer dependency? If the goal is to avoid bundling it in the built output, I think that should suffice, and makes it easier for the downstream consumer vs. separately installing the i18n package?

@allthesignals
Copy link
Contributor Author

I wonder -- could it work to just mark the package as an external and list it as a dependency rather than a peer dependency? If the goal is to avoid bundling it in the built output, I think that should suffice, and makes it easier for the downstream consumer vs. separately installing the i18n package?

Oh, yeah, let me try that.

(For my own understanding:) IIRC we moved react/react-dom to peer dependencies because react itself is pretty strict about which copy of react is imported and used to render...? I'm a little fuzzy on the reasoning. Like, the consumer has to import react to render, and we need our library to reference that same copy. (The alternative, letting it implicitly load as a regular dependency, wouldn't allow us to import react consumer-side at all... but then why can't npm/yarn resolve this correctly to only use one copy? Brushing up here).

@aduth
Copy link
Contributor

aduth commented Sep 25, 2023

Yeah, that's a good question. I think in that case it has a lot to do with the downstream project also needing to use a React dependency, so I suppose similar logic could apply for i18n if we're expecting to use it directly in identity-site.

It gets more fuzzy since in either case Webpack or whatever bundler should be deduplicating regardless. 🤷

@allthesignals
Copy link
Contributor Author

Yeah, that's a good question. I think in that case it has a lot to do with the downstream project also needing to use a React dependency, so I suppose similar logic could apply for i18n if we're expecting to use it directly in identity-site.

Yeah, actually, I was about to say... we do directly import and use i18n on identity-site, and they need to be the same copy. So, I don't think we can get away with having it listed as a regular dependency.

It gets more fuzzy since in either case Webpack or whatever bundler should be deduplicating regardless. 🤷

Right, it's weird Webpack wasn't doing that. It included 3 copies instead of 1! Maybe that has to do with how the externals feature works.

Copy link
Contributor

@aduth aduth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given prior discussion, I think this works as-is 👍

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.

3 participants