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

Plugin system #2756

Closed
wants to merge 26 commits into from
Closed

Plugin system #2756

wants to merge 26 commits into from

Conversation

Timer
Copy link
Contributor

@Timer Timer commented Jul 10, 2017

I'd like to preface this by stating that it is very likely a system like this may never be added to create-react-app, and is simply being showcased to create discussion around the feasibility of such an idea.

A plugin system is something which proposes to solve many problems.

For instance, we'd like to be able to add support for things like Relay, without increasing the initial installation size of the build tooling.

This could also potentially be applied to things like TypeScript, Sass, et al.

Design goals:

  1. Zero-configuration, opt-in additional features
  2. Simple setup (yarn add react-scripts-plugin-relay)
  3. Transparent to users who eject

Design constraints:

  1. Do not increase the initial installation size with excess modules
  2. Under no circumstances retain a "plugin" system when ejecting; everything must be flattened into a single file

This PR introduces a POC plugin system with initial support for TypeScript.

TODO

  • Install plugin dependencies when ejecting
  • Add eject step for plugins (copy files [e.g. tsconfig.json], etc)

@Timer
Copy link
Contributor Author

Timer commented Jul 10, 2017

So, what's going on here?

Glad you asked!

Plugins exist as packages prefixed by react-scripts-plugin-*; these packages export a single function, apply .

apply takes our webpack configuration file and required scoped variables, and then mutates the configuration (impure; the object not the file).

Any mutations must be applied using helpers exported by react-dev-utils/plugins, which are currently restricted to:

  1. pushExtensions: adds extensions to be resolved
  2. pushExclusiveLoader: adds a new exclusive loader to the configuration after a specified loader

These mutations are applied to our dev and prod webpack configuration files at runtime while not ejected:

module.exports = applyPlugins(base, ['typescript'], { paths });

When ejecting, these mutations must happen to the configuration files pre-emit.
We do not want ejected users to be subjected to a plugin system, which defeats us "giving" them full control of the configuration.

To achieve this, the plugins are parsed and placed into the configuration file to remove the seams. 😄


tl;dr Magic! ✨ 🔮

@Timer
Copy link
Contributor Author

Timer commented Jul 10, 2017

/cc @viankakrisna who was interested in seeing this prototype

@dashed
Copy link

dashed commented Jul 10, 2017

I'm hoping this would eventually enable server-side rendering 😄

@Timer
Copy link
Contributor Author

Timer commented Jul 10, 2017

Missing installing plugin dependencies, but you get the idea:

$ cd app1/
$ echo y | yarn eject
$ git add -A; git commit -m "normal"
$ cd ../app2/
$ yarn add react-scripts-plugin-typescript # All that's required to add typescript support, pre and post eject
$ echo y | yarn eject
$ cp -r . ../app1/
$ cd ../app1/
$ git diff | pbcopy
diff --git a/config/webpack.config.dev.js b/config/webpack.config.dev.js
index eaabb8a..42ea5e7 100644
--- a/config/webpack.config.dev.js
+++ b/config/webpack.config.dev.js
@@ -84,7 +84,7 @@ module.exports = {
     // https://github.com/facebookincubator/create-react-app/issues/290
     // `web` extension prefixes have been added for better support
     // for React Native Web.
-    extensions: ['.web.js', '.js', '.json', '.web.jsx', '.jsx'],
+    extensions: ['.web.js', '.js', '.tsx', '.ts', '.json', '.web.jsx', '.jsx'],
     alias: {
       
       // Support React Native Web
@@ -153,6 +153,15 @@ module.exports = {
               cacheDirectory: true,
             },
           },
+          {
+            // Process TypeScript with `at-loader`
+            test: /\.(ts|tsx)$/,
+            include: paths.appSrc,
+            loader: require.resolve('awesome-typescript-loader'),
+            options: {
+              silent: true, 
+            },
+          },
           // "postcss" loader applies autoprefixer to our CSS.
           // "css" loader resolves paths in CSS and adds assets as dependencies.
           // "style" loader turns CSS into JS modules that inject <style> tags.
diff --git a/config/webpack.config.prod.js b/config/webpack.config.prod.js
index 7d4649e..fe0ef64 100644
--- a/config/webpack.config.prod.js
+++ b/config/webpack.config.prod.js
@@ -86,7 +86,7 @@ module.exports = {
     // https://github.com/facebookincubator/create-react-app/issues/290
     // `web` extension prefixes have been added for better support
     // for React Native Web.
-    extensions: ['.web.js', '.js', '.json', '.web.jsx', '.jsx'],
+    extensions: ['.web.js', '.js', '.tsx', '.ts', '.json', '.web.jsx', '.jsx'],
     alias: {
       
       // Support React Native Web
@@ -151,6 +151,15 @@ module.exports = {
               compact: true,
             },
           },
+          {
+            // Process TypeScript with `at-loader`
+            test: /\.(ts|tsx)$/,
+            include: paths.appSrc,
+            loader: require.resolve('awesome-typescript-loader'),
+            options: {
+              silent: true, 
+            },
+          },
           // The notation here is somewhat confusing.
           // "postcss" loader applies autoprefixer to our CSS.
           // "css" loader resolves paths in CSS and adds assets as dependencies.
diff --git a/package.json b/package.json
index 1239483..4ba74fc 100644
--- a/package.json
+++ b/package.json
@@ -1,5 +1,5 @@
 {
-  "name": "app1",
+  "name": "app2",
   "version": "0.1.0",
   "private": true,
   "dependencies": {

Off to bed now, night!

try {
return require.resolve(`react-scripts-plugin-${p}`);
} catch (e) {
return null;
Copy link
Contributor

Choose a reason for hiding this comment

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

Any expected errors that can happen here?

Copy link
Contributor Author

@Timer Timer Jul 10, 2017

Choose a reason for hiding this comment

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

Plugin installation is optional, so require.resolve throws an error if the package is missing.

@viankakrisna
Copy link
Contributor

viankakrisna commented Jul 10, 2017

Nice! it's like neutrino presets, but ejectable. :D

Maybe on the next iteration, we can detect if the user is importing .scss file, we ask them do they want to install a react-scripts-plugin-sass and install it. Magic! ✨ 🔮

Another thing is where the plugins repo would live? Can we maintain a list of the blessed react-scripts plugins? If not, what's stopping people to writing plugin that expose overriding all react-scripts configuration?

It's really exciting!

@viankakrisna
Copy link
Contributor

(and frightening at the same time)

@Arrow7000
Copy link

Arrow7000 commented Jul 10, 2017

Hi, sorry if this suggestion is silly or what I'm saying is unclear - I'm still a fairly junior developer. But here's my two cents:

Personally I don't like the idea of adding/removing plugins mutating the global webpack configuration. It opens users up to all sorts of permanent errors - all it takes is one buggy plugin to corrupt things permanently.

What about having plugins just insert a single file into a plugins folder, and put a hook in the webpack config that has it go through every file in that folder? That would obviate the need for having an apply (and presumably a remove function) in every plugin, as nothing would be mutated. Webpack would just take part of its configuration from the files in a folder.

I see this as working in a similar way to how Jekyll loads plugins, by simply running every file in the _plugins folder - no configuration required.

@viankakrisna
Copy link
Contributor

@Arrow7000 that's why I think it's frightening, but if CRA maintainer can control the plugin code, and maintain a whitelist of allowed plugins, they can curate the experience and provide bug fixes early.

@Arrow7000
Copy link

@viankakrisna yes curation will help prevent the problem, but if webpack configs are being mutated then one bug in a plugin can ruin your config, regardless of any subsequent bug fixes to the plugin.

@viankakrisna
Copy link
Contributor

Then we need to write test of the playability of each plugin that we have. And maybe detect if the plugin version needs to be updated? that's why the core should have a list of installable plugins (and it's compatible version)

@Timer
Copy link
Contributor Author

Timer commented Jul 10, 2017

@viankakrisna

Another thing is where the plugins repo would live? Can we maintain a list of the blessed react-scripts plugins? If not, what's stopping people to writing plugin that expose overriding all react-scripts configuration?

Plugins will live within the monorepo; for now (and probably indefinitely) plugins are whitelisted so people cannot create their own.

Then we need to write test of the playability of each plugin that we have. And maybe detect if the plugin version needs to be updated? that's why the core should have a list of installable plugins (and it's compatible version)

Yeah, testability is key here. Playability is a worry as we upgrade react-scripts -- so maybe the whitelist should include the version -- I like that idea!

@Timer
Copy link
Contributor Author

Timer commented Jul 10, 2017

@Arrow7000

Personally I don't like the idea of adding/removing plugins mutating the global webpack configuration. It opens users up to all sorts of permanent errors - all it takes is one buggy plugin to corrupt things permanently.

The plugins will never actually mutate the file unless you are ejecting, so there is no potential for permanent errors.
Plugins will be whitelisted as well, meaning only plugins we create can be used. This is so we can keep curating the user experience and providing painless upgrades.

This impl is contingent on updating remaining as simple as it is now.

What about having plugins just insert a single file into a plugins folder, and put a hook in the webpack config that has it go through every file in that folder? That would obviate the need for having an apply (and presumably a remove function) in every plugin, as nothing would be mutated. Webpack would just take part of its configuration from the files in a folder.

Like above, installing the plugins themselves and uninstalling will not actually cause any true mutations -- mutation only happens when ejecting and is intended to be permanent.

Hi, sorry if this suggestion is silly or what I'm saying is unclear - I'm still a fairly junior developer. But here's my two cents:

Thanks for chiming in and I wish I was a little more clear off the bat!
Your concerns are very valid, so give yourself more credit! (p.s. never apologize for being a junior & you should always be asking all kinds of questions to further your knowledge + understanding 😄; feel free to hit me with more)

@Timer
Copy link
Contributor Author

Timer commented Jul 10, 2017

@dashed

I'm hoping this would eventually enable server-side rendering 😄

Perhaps! Maybe you could contribute support if a system like this was put into place.

@nicksarafa
Copy link

If we could extend plugins to implement @decerators it would've saved our team some huge headaches when implementing MobX. This feature has our 👍

@Timer
Copy link
Contributor Author

Timer commented Jul 10, 2017

@nsarafa

If we could extend plugins to implement @decerators it would've saved our team some huge headaches when implementing MobX. This feature has our 👍

Unfortunately, we will not allow the creation of custom plugins (or at least not yet).
Custom plugins are something that Neutrino is highly specialized for, so I suggest you check that out.

We have no plans of enabling decorators (or any proposal/language feature for that matter) until they reach stage-3 and become commonplace.

Sorry!

@Timer
Copy link
Contributor Author

Timer commented Jul 13, 2017

Proposal now installs plugin dependencies during eject.
TypeScript now works pre and post eject with no fuss (assuming valid tsconfig.json post eject)!

Next step is to automate tsconfig.json updating during eject via a plugin eject function.

@Timer
Copy link
Contributor Author

Timer commented Jul 13, 2017

I'd love some review, feedback, or suggestions!

@Timer
Copy link
Contributor Author

Timer commented Jul 13, 2017

Closing this in favor of #2784.

@Timer Timer closed this Jul 13, 2017
@Timer Timer deleted the typescript branch July 13, 2017 18:31
@viankakrisna viankakrisna mentioned this pull request Jul 14, 2017
@lock lock bot locked and limited conversation to collaborators Jan 21, 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.

7 participants