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

Require monorepo source workspace packages to be opted in via glob. #4570

Closed
wants to merge 3 commits into from

Conversation

bradfordlemley
Copy link
Contributor

The current 2.0 implementation treats all monorepo packages included by the app as source packages, there is no way to opt them in or out, and this results in making some monorepos incompatible with react-scripts (see issues listed below).

This PR changes the behavior to require an app to opt source packages in via a glob pattern ("craSourceWorkspaces") in the app's package.json, similar to the "workspaces" glob pattern used by yarn to opt packages in to workspaces.

(This is an alternative implementation to the #4092 proposal.)

Fixes #4569, #4410, #4249.

@gaearon
Copy link
Contributor

gaearon commented Jun 5, 2018

Naming aside (cra prefix won't mean anything in an ejected app so I'd prefer to drop it), I'm concerned about these issues:

  • I want to support non-workspace source-only packages (which seems like a common need).
  • It's counter-intuitive that renaming a package changes its "compiled" status. With sourceDependencies we can validate their names and throw if there isn't an exact match for every item in dependencies or peerDependencies. So you know renaming won't break it silently.

@bradfordlemley bradfordlemley force-pushed the mono-optin branch 4 times, most recently from fb14f19 to 3fa3cfe Compare June 7, 2018 12:55
@bradfordlemley
Copy link
Contributor Author

I want to support non-workspace source-only packages

I'm not opposed to supporting the private npm registry use case and I can see how a unified solution would be desireable; however, if supporting the other cases sacrifices usability for the monorepo case, then I think it's ok to support them via different mechanisms.

I think the most common use case will be a monorepo w/ shared source + transitive source dependencies. With this solution, the configuration is very easy. With the #4092 solution, it's a lot more configuration, particularly for transitive source dependencies.

It's counter-intuitive that renaming a package changes its "compiled" status

Moving the package changing its compiled status should be expected if the sources are specified via paths -- I don't think that would be a difficult issue to understand or debug.

In general, specifying by path globs in a monorepo is a really user-friendly and flexible option. You can configure globs such that new packages in certain directories automatically get compiled without any additional config. Or you can configure without wildcards so that you have to specify each package if that's how you want to do it.

It would be possible to support wildcards in the #4092 proposal, and I'm not opposed to doing that either, but that would be with package names instead of paths, so it's still a different dimension that what's desirable for monorepos.

Summary:

  • Configuration by path globs makes a lot of sense in a monorepo.
  • RFC: Source Packages #4092 could be used to support the private npm repository case.

@Timer
Copy link
Contributor

Timer commented Sep 26, 2018

We took this out of 2.0 for now, maybe in 3.0, though! This complexity was too much to manage.

Let's meet back in #4092.

@Timer Timer closed this Sep 26, 2018
bradfordlemley added a commit to bradfordlemley/create-react-app that referenced this pull request Oct 11, 2018
bradfordlemley added a commit to bradfordlemley/create-react-app that referenced this pull request Nov 1, 2018
@lock lock bot locked and limited conversation to collaborators Jan 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants