facebook / create-react-app

Set up a modern web app by running one command.
https://create-react-app.dev
MIT License
102.8k stars 26.87k forks source link

RFC: Source Packages #4092

Open bradfordlemley opened 6 years ago

bradfordlemley commented 6 years ago

RFC: Source Packages

A developer-, tool-, and ecosystem- friendly mechanism for sharing source.

co-authored with @gaearon

Overview

"Source packages" are the same as standard npm packages, except they may contain non-standard language features (e.g. React JSX) and are built by the consumer.

"Source packages" are included as standard dependencies in package.json and declared as source by also including them in sourceDependencies:

package.json
{
  "name": "myapp",
  "dependencies": {"pkg1": ">0.0.0"},
  "sourceDependencies": ["pkg1"]  // declare as source package
}

Being standard npm packages, source packages can be managed by standard tools and can be truly modular since they can declare their own dependencies.

Being built by the consumer, the consuming build can provide the same build features and developer experience for source packages (transpiling, hot-reloading, de-duping, etc.) as it does for its own source.

Since source packages may contain non-standard language features, they should be marked as "private". They can be contained in monorepos and/or published to private registries.

Source packages should be testable by the consumer, just like the consumer's own source. This facilitates concurrent development of shared components.

This proposal does not include a mechansim for a source package to describe its source code, e.g. which language features it uses. The proposal assumes that the consumer knows which source packages it is including and is able to build them, e.g. that the included source packages have the same build requirements as the consumer's own source.

Pseudo-algorithm for finding source packages

sourcePackages = Map()

FindSourcePkgs(package.json):
  for each dep in package.json dependencies
    if dep is also in package.json sourceDependencies
      resolve dep package path
      if dep package.json has private flag
        sourcePackages[dep] = dep package path + dep package.json source
        FindSourcePkgs(dep package.json)

FindSourcePkgs(initial package.json)

Supports:

Example

This repo demonstrates many of the use cases supported by this proposal.

repo/
  app1/
    package.json
      name: "app1"
      dependencies: {
        "comp1": ">0.0.0",
        "kewl-comps": ">0.0.0"
      }
      sourceDependencies: [
        "comp1",         // single module in source package
        "kewl-comps"     // multiple modules in source package
      ]
    src/
      App.js
        import comp1 from 'comp1';
        import kewl1 from 'kewl-comps/comp1';
  comp1/
    package.json
      name: "comp1"
      dependencies: {
        "comp2": ">0.0.0"
      }
      sourceDependencies: [
        "comp2"         // transitive source dependency
      ]
      private: true
    index.js
      import comp2 from 'comp2';         
      import intLib from './int-lib'; // internal transitive source dependency
    int-lib.js
  comp2/
    package.json
      name: "comp2"
      private: true
    index.js
  kewl-comps/
    package.json
      name: "kewl-comps"
      dependencies: {
        comp5: ">0.0.0"
      }
      sourceDependencies: [
        comp5
      ]
      source: "src"  // source entry point
      private: true
    src/
      comp1.js
        import comp2 from './comp2'; // internal transitive source dependency
        import comp5 from 'comp5';
      comp2.js
    node_modules/
      comp5/  // source dependency from private registry
        package.json
          name: "comp5"
          dependencies: {
            "comp6": ">0.0.0"
          }
          sourceDependencies: [
            "comp6" // transitive source dependency
          ]
          source: "src"
          private: true
        src/
          index.js
            import comp6 from 'comp6';
        node_modules/
          comp6/  // nested transitive source dependency from private registry
            package.json
              name: "comp6"
              private: true
            index.js
yyfearth commented 6 years ago

This is a very cool suggestion. It solved the problem which monorepos with workspace cannot always solve. Esp. for code that is sharing across projects, which have their own repos or monorepos. Since you cannot put all your companies code into a single monorepo.

But for the source package, the problem is how to ensure the packages has the same language/feature configurations as the consumer. Or how to provide the configuration to the consumer, which is possible, e.g. .babelrc.

Also why the private has to be true? If we can find a way to provide the configuration, or we provide the built version with the source, we can publish it. Furthermore, for any packages which includes the source code, we can use it as the source dependency.

In my own case, we have few npm packages which publish to our private npm registry. They are React components, which can be reused across most of our React apps. Instead of use the compiled/bundled version, we use the source code directly from the apps, so it can produce smaller/better bundle. And for projects using different compile configuration, which is not compatible, then we use the compiled/bundled version by default.

bradfordlemley commented 6 years ago

how to ensure the packages has the same language/feature configurations as the consumer

This proposal is aimed at the common use case of sharing "like" components across "like" consumers (i.e., sharing React (JSX) components among React apps), essentially providing a way to use external packages as if they were contained within the consumer's own source tree.

I, too, would like to see a solution that could handle arbitrary source packages, but that adds a lot of complexity, and is currently outside the scope of this proposal.

why the private has to be true

The main argument for requiring "private" flag is to protect the ecosystem from non-standard packages. The other argument is that publishing necessarily requires the package to have its own build system, so it should build the package anyways for consistency. Looking at it this way, using a package as a source package (as defined in this proposal) and publishing it could be considered conflicting goals. That said, it seems legit to want the benefits of source packages in your internal projects and also the ability to responsibly publish the package for public consumption. I could be convinced to drop the private flag requirement, but could @gaearon be convinced?

gaearon commented 6 years ago

Related, might want to post some feedback https://github.com/parcel-bundler/parcel/pull/1101

SavePointSam commented 6 years ago

What prevents detection of Babel settings in the alien package? I would imagine it could be handy to detect some Babel config and run it against the alien source, but still falling back to local configuration upon failure or missing configuration. Seeing as you could have an alien package rely on yet another alien package not in the local dependency tree, the initial alien’s dependencies will need to be present regardless of an alien transpile step. At the same time I understand the complexity this adds to the development runtime. Two or more unique listeners for isolated transpilation steps would be tricky.

I do agree for the initial implementation it will be worthwhile to only support transpilation from the local configuration. At the very least it will allow for quicker implementation and therefore higher likelihood to land in 2.0.

The first step toward implementation would be verification of webpack’s support for the source field. I see that parcel has added support for the source field as of 1.8.0.

Also, as there doesn’t appear to be a lot of traffic on this RFC. I would like to suggest we nail it down soon as I would like to start implementing this solution for linked monorepo support ASAP.

bugzpodder commented 6 years ago

babel 7b46 has support for babel.config.js https://github.com/babel/babel/pull/7358

gaearon commented 6 years ago

What prevents detection of Babel settings in the alien package?

It requires everyone to agree on versions. Now you can't upgrade to React Scripts 3 using Babel 8 because your other package happens to use Babel 7.

Not a fan of attempting to compile anything with its own config.

SavePointSam commented 6 years ago

Very valid thought process there. Let’s stick with the consumer being responsible to have a config that compiles all alien code then.

Is there anything else we need to discuss here?

gaearon commented 6 years ago

Not sure—this just needs someone to drive the changes and implement them.

gaearon commented 6 years ago

We'll also need to look at what Parcel is doing in case they already tackled this.

SavePointSam commented 6 years ago

I'd be happy to lead the effort for this since I'm a valid use case and want to help with CRA 2 anyway :D

bradfordlemley commented 6 years ago

I've proposed another solution in #4570: a simple glob pattern to specify source packages, similar to how yarn "workspaces" are specified. I find that option to be more simple and developer-friendly, and this proposal to be overly complex in comparison.

IMO, the goal should be: add the ability to spread the source out in a monorepo so it can easily be shared amongst multiple apps in the monorepo. And it would be good to keep it as simple as possible.

Generic source package support (ie., allowing packages to specify their own build config) is being addressed by some other tools (parcel-bundler/parcel#1101), and I think that is a different problem than what CRA is trying to solve here. Generic source package support would also be great, but I think that can be considered as a separate feature. My concern with this proposal is that it seems to be crossing the two features and making things unnecessarily complicated. (Yes, my finger is pointing at myself.)

SavePointSam commented 6 years ago

It is my understanding that this RFC is strictly about allowing support for transpiling source code in another package as made available by the source key. It isn't directly required to be a monorepo only solution. A consumer could have a private NPM package that is maintained outside a monorepo setting that would still be able to take advantage of this pattern. At the same time I would highly suggest against it as keeping babel settings consistent between the packages would be awful.

I think it would be worthwhile to consolidate all the monorepo pain points into a single effort and this being piece of that puzzle.

gaearon commented 6 years ago

A consumer could have a private NPM package that is maintained outside a monorepo setting that would still be able to take advantage of this pattern.

👍 that was my concern too, it seems like people increasingly do this with private registries

gaearon commented 6 years ago

Anecdotal but: https://mobile.twitter.com/dan_abramov/status/1004122380036444163

We should support this.

MilosRasic commented 6 years ago

I feel more tooling support for complex setup like this is definitely needed, but I'm not sure CRA is the place to solve the problem. It's intended to quickly bootstrap React apps with zero config. Is there value in making it more complicated when someone with a complex setup is definitely going to want to eject at some point?

Regardless of my misgivings, here's some notes from my experiences with a complex project in case you decide to go on with the effort:

There's no need to focus on monorepo. It's cool because Facebook and Google and babel do it, but multirepo setup is perfectly valid, sometimes necessary, and faces the exact same issues: building a single app from multiple private packages. Whether packages are published from one or multiple repos doesn't make much of a difference.

I'm not convinced package.json is the best place for new config. package.json is already bloated with non-standard entries used by many libraries and tools. It also adds another place where you have to manually add a package when you add it to the project. I might be misunderstanding how the raw packages are going to be processed, but it seems to me what is needed is to include them in babel-loader even though they are in node_modules. Of course, we can't have an include or exclude function in package.json, but maybe an array of regular expressions would work? On the project I'm working on at Gogo we opted to have a prefix for private packages, but a namespace would work too.

Supporting both yarn-linked and non-linked setup is tricky. Webpack is going to resolve symlinks created by yarn link to the absolute paths of their targets. Our include function looks like this:

function(modulePath) {
    var escapedPath = path.resolve('..').replace(/\\/g, '\\\\');

    var include = modulePath.startsWith(path.resolve('src')) ||
        (modulePath.includes('node_modules' + path.sep + APP_PREFIX) &&
        !modulePath.match(APP_PREFIX + '.*\\' + path.sep + 'node_modules')) ||
        modulePath.match(escapedPath + '\\' + path.sep + '[a-zA-Z-]+' + '\\' + path.sep + 'index.js') ||
        modulePath.match(escapedPath + '\\' + path.sep + '[a-zA-Z-]+' + '\\' + path.sep + 'src' + '\\' + path.sep + '.*');

    return include;
}

I'm sure everyone has seen nicer code. Also please excuse my es5, this was originally written for node 4 iirc.

If you are not using a test runner that can replicate settings from webpack config, and I have no idea if jest can, you have to have a similar function with @babel/register manually called before tests are ran.

SavePointSam commented 6 years ago

I feel more tooling support for complex setup like this is definitely needed, but I'm not sure CRA is the place to solve the problem. It's intended to quickly bootstrap React apps with zero config.

You could say the same thing about many features that CRA already supports. Proxy, Flow, and even CSS Modules or Sass. However, as time moves on so does CRA. Those tools are all now supported, they were perfectly possible through ejecting. I would argue that CRA is a place to build react apps, not just bootstrap the minimal amount of things possible to make a react app. Therefore supporting monorepos makes sense to me. Why not support a tool that the tool itself uses?

Either way, this RFC is about supporting the transpiling of external/alien sources, not specifically in a monorepo. For the initial implementation we have agreed that we will not clutter things by supporting extra babel/webpack configuration that the alien source may need. At that point the alien source should have it's own build step anyway.

I'm not convinced package.json is the best place for new config. package.json is already bloated with non-standard entries used by many libraries and tools.

This RFC is not about adding anything to the package.json any further other than supporting the source key in the package.json. A key that we didn't make up, but borrowed from existing tooling that implemented that key for the sake of this type of work and already solved that problem. At the same time, that key doesn't go in the application package.json, it belongs to the alien package that is allowing a consumer to say 'hey, here is the entry to my source, try and transpile it if you like.'

Granted, we may need to do something to flag this setting on, as I don't think it should be enabled by default. Maybe adding a regex based key to the consumer does make sense so that users can whilelist the packages they want to transpile. However, we are still transpiling node_modules now anyway. Certainly up for discussion still.

Ultimately, we need to pare down what all we support in the initial implementation for sure. I want to avoid adding as much as possible to the developer guide for this support. It will also allow the work to me incremental and if someone needs something further in the future it can be discussed.

For now, this proposal to add support for using the source key in an alien package makes sense and is very minimal. We even have proof that people want it (aside from myself). It will work for both private registry packages and monorepos as it is outside the exact module system and simply supports a new entry point as defined in the alien package.

bradfordlemley commented 6 years ago

This RFC is not about adding anything to the package.json any further other than supporting the source key in the package.json.

That's not correct. The main functionality of this RFC lies in sourceDependencies which is the mechanism to specify which packages should be treated as source. As currently specified, sourceDependencies has to be present in the app's package.json AND in any source dependency's package.json that also has source dependencies ("transitive source dependencies").

SavePointSam commented 6 years ago

Oh, my mistake. Well, I'm not opposed to that either. Though we could bikeshed the key name. I even alluded to that functionality in my last post.

Maybe adding a regex based key to the consumer does make sense so that users can whilelist the packages they want to transpile.

I suppose I just glossed over that piece when writing my response. Even still, being able to whitelist source packages isn't locked into monorepos like you've stated in your proposal.

bradfordlemley commented 6 years ago

Anyone have feedback on the sourceDependencies mechanism which is used for specifying which packages should be treated as source?

In the current proposal, each source dependency must be explicitly listed in the app's sourceDependencies and must match a dependency listed in its dependencies -- there is no support for wildcard, regex, etc in sourceDependencies.

Additionally, transitive source dependencies must be specified by the package that includes them, which is a little awkward because the dependencies then control what the app builds.

These mechanisms seem cumbersome, particularly in a monorepo -- it would be much easier to have the ability to specify the entire monorepo, or parts of it, via wildcards (like #4570) and I think that would be the most common use case.