eliminate dev-only code on eject #53 #191

[Jiansen]

// Dead code on eject: start
dev-only code
// Dead code on eject: end

will be removed when the scripts are exposed to users on eject

npm pack copies all files to the created package. To adopt the approach proposed in #191 and give a clean code in the published package , we may need to temporary add files to .npmignore before npm, similar to what we do to package.json code link

[ghost]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

[Jiansen]

will look at the test problem

[ghost]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[Jiansen]

I find the reason why CI build fails.

After the eject, the --smoke-test flag is removed, thus e2e doesn't complete the last test on eject and build.

@gaearon ,perhaps the --smoke-test flag is useful for users to test the created app and worth to be kept after eject. What do you think?

[gaearon]

I don’t think --smoke-test is valuable to ejected users.
It’s only meant for testing our configuration.

What do you think is a reasonable solution here?

[Jiansen]

@gaearon ,

I don't have a good solution now. I am thinking if there is an alternative way to test the _eject_ed start script.

BTW, it looks that --smoke-test is not used to load configuration at this moment, it is used to terminate the server. Do you plan to do extras in --smoke-test, as the name suggests?

if (isSmokeTest) {
  handleCompile = function (err, stats) {
    if (err || stats.hasErrors() || stats.hasWarnings()) {
      process.exit(1);
    } else {
      process.exit(0);
    }
  };
}

Perhaps, we could make a --start-test flag available to users, but reserve the --smoke-test flag for internal use only,

--start-test: the server can be started, exit with signal 1 if error/warning occurs (console.log those errors/warnings ?), otherwise exit with signal 0.
--smoke-test: start-test + other tests that only make sense during the internal development process (i.e. using different configurations).

[gaearon]

BTW, it looks that --smoke-test is not used to load configuration at this moment, it is used to terminate the server. Do you plan to do extras in --smoke-test, as the name suggests?

What I meant is that, with this flag, we can run npm start and make sure it doesn’t crash with dev configuration, and that Webpack didn’t output any errors.

This is not useful to users because they can run npm run build for the same purpose. We specifically need to test dev configuration only because we want to make sure we don’t break it by accident as we work on this tool. Real users don’t need this because they aren’t exposed to the configs.

[Jiansen]

This is not useful to users because they can run npm run build for the same purpose.

Yes, it is straightforward to see if there is any errors/warnings in local console, and any problem of starting server is obvious to be noticed when changes are made locally. But if users want to add CI Build to their react project, as what we did here, it might be worth to check the project can be started when installed in a clean environment (it usually takes a long time to do a clean re-install locally).

An idea just come to my mind when typing the above. Following your plan in #191 , we can import handleCompile in a separate file (it is available in this project and e2e will pass), but .npmignore the file (handleCompile is undefined in released package).

[gaearon]

But if users want to add CI Build to their react project, as what we did here, it might be worth to check the project can be started when installed in a clean environment (it usually takes a long time to do a clean re-install locally).

If it can be built, it can be started. (At least that’s something we want to enforce.) Unless we screw up our configs, all buildable projects should also be startable.

we can import handleCompile in a separate file (it is available in this project and e2e will pass), but .npmignore the file (handleCompile is undefined in released package).

This will likely be confusing and error prone. Let’s just keep handleCompile (and the knowledge of that flag) for now.

[Jiansen]

I agree with you.

So let's leave the PR open for now. I will play around and try to develop a way to "ship dev-only or dep-only code" as you proposed in #191 .

Before releasing the next version,

• if we could do a "dep-only" ship, let's do that
• if we cannot do it nicely somehow, then use the method in this PR to clean paths.js on eject, but keep the start.js intact

[gaearon]

Closing in favor of #257.