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

Clean publish and eject #257

Merged
merged 1 commit into from
Sep 2, 2016
Merged

Conversation

Jiansen
Copy link
Contributor

@Jiansen Jiansen commented Jul 28, 2016

Disscussion 1: add parameter stage to pack.sh so that

  • pack.sh prod only ship prod configurations
  • pack.sh dev only ship dev configurations
  • pack.sh test only ship test configurations
  • etc.

This can be done by removing files in package.json, and creating a few versions of .npmignore files (e.g. .npmignore.prod).

Disscussion 2: further split paths.prod.js to 2 files, paths.beforeEject.js and paths.afterEject.js; remove the // Dead code on eject:start/end block.

Question:
start.js always use the dev config for webpack. Is there a reason for this? In my local test, it seems fine to use production config in a generated app.

@ghost ghost added the CLA Signed label Jul 28, 2016
@Jiansen
Copy link
Contributor Author

Jiansen commented Jul 28, 2016

Don't merge for now.

I found this in CI build console. e2e isn't completed there. Might suggest another issue to fix.

++++tasks/pack.sh
tasks/e2e.sh: line 50: tasks/pack.sh: Permission denied

@vjeux
Copy link
Contributor

vjeux commented Jul 28, 2016

You probably want to chmod +x tasks/pack.sh

@Jiansen
Copy link
Contributor Author

Jiansen commented Jul 28, 2016

@vjeux ,

Yes, I checked the permission on my local machine, it has +x permission. git status doesn't show anything to commit. If permission change is not picked by git, I will probably delete and re-add the pack.sh file.

Thankfully, the last build (222.1) looks OK.

@ghost ghost added the CLA Signed label Jul 28, 2016
@@ -11,7 +11,7 @@ var path = require('path');
var autoprefixer = require('autoprefixer');
var webpack = require('webpack');
var HtmlWebpackPlugin = require('html-webpack-plugin');
var paths = require('./paths');
var paths = require('./')('paths');
Copy link
Contributor

Choose a reason for hiding this comment

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

I’m worried that this convention is going to look super confusing to people after ejecting (or even for those who want to contribute to this project). What alternative solutions have you considered?

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 agree that the code is confusing to some extent. In fact, I leant very recently that require looks for index.js when the required path is a folder.

Two ideas:

  1. replace it with require('../config')('paths'); even if the file is in the same folder. At least the word config appears in the code to suggest the meaning of this line.
  2. create a symbolic name config so that the above code is rewritten as require('config')('paths').

I don't know how to do 2 honestly. I will take a look at this method and search for others.

There is another decision worthy a discussion: should config/index.js export a function (like the current change) or an object with fields like paths and so on.

It is more conventional for a module to export an object or a constructor. But exporting a function has advantages in this case:

  1. the code is short, and we don't need to introduce a new field every time when a new configuration is introduced; simply create a *.prod|dev.js file.
  2. we could add an error message/exception when developers made a typo on importing a configuration, let the typo be exposed earlier.
  3. in test/dev, we are still able to import a prod config (e.g. the current build.js always uses webpack.prod)

@gaearon
Copy link
Contributor

gaearon commented Jul 29, 2016

Looking at this, I think we should keep it simple and not mess with the filenames.
If paths.js is the only thing that really needs branching, let’s just do something like

// paths.js

module.exports = {
  // ejected paths
}

// @remove-on-eject-begin
if (...) {
  module.exports = // local dev paths
} else if (...) {
  module.exports = // dependency paths
}
// @remove-on-eject-end

@Jiansen
Copy link
Contributor Author

Jiansen commented Jul 29, 2016

If paths.js is the only thing that really needs branching ...

Changes in this PR cut off code at 2 places

  1. when pack/publish the project as a npm module
  2. when a user runs eject

If we don't do 1, then dev-only flags like --debug-template are still available to users, though they need to dig a bit into the source code of this open source project :p

I like your syntax of // @remove-on-eject-begin/end. I will refactor code in PR #206, which only does 2. We could review if features in the top comment are needed or not, and update the code accordingly.

@gaearon
Copy link
Contributor

gaearon commented Jul 29, 2016

We can do (1) with separate flags.
Maybe something like:

// paths.js

module.exports = {
  // ejected paths
}

// @remove-on-publish-begin
if (...) {
  module.exports = // local dev paths
}
// @remove-on-publish-end

// @remove-on-eject-begin
if (...) {
  module.exports = // dependency paths
}
// @remove-on-eject-end

@Jiansen
Copy link
Contributor Author

Jiansen commented Jul 29, 2016

Yes, merging *.prod.js file and *.dev.js into one file is a good idea for code maintenance.

I will try later today or over this weekend. Thanks for your advice.

@gaearon gaearon added this to the 0.3.0 milestone Jul 29, 2016
@ghost ghost added the CLA Signed label Jul 29, 2016
@ghost ghost added the CLA Signed label Jul 31, 2016
@ghost ghost added the CLA Signed label Jul 31, 2016
@Jiansen
Copy link
Contributor Author

Jiansen commented Jul 31, 2016

Refactored and rebased previous changes as suggested. To summarise

  • tag dev-only code with // @remove-on-publish-begin/end tag
  • tag pre-eject-only code with // @remove-on-eject-begin/end tag

As per the support of the above 2 tags, if code looks OK, this PR is ready to be merged.

If we wanted to merge babel.[prod|dev].js and webpack.config.[prod|dev].js as well, I have 2 questions

  1. *.prod.js contains some code that is not in *.dev.js
  2. Currently build.js always uses webpack.config.prod whereas start.js always use webpack.config.dev
// `build.js`
var config = require('../config/webpack.config.prod');
// `start.js`
var config = require('../config/webpack.config.dev');

In an earlier test, it seems fine to let start.js uses prod config in production environment. I haven't tested if build.js could use dev config in local development environment.

I will search for webpack related issues and PRs to understand why those two files are tied to specific configurations (runtime/build efficiency?). If someone has a short answer, or knows the reference to a related issue/PR, please let me know.

# Exit the script on any command with non 0 return code
# We assume that all the commands in the pipeline set their return code
# properly and that we do not need to validate that the output is correct
set -e
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please use the same error handling mechanism that e2e test now uses?

@gaearon
Copy link
Contributor

gaearon commented Aug 2, 2016

I left a few final comments, I think it’s good to go when they are addressed.
Also please rebase, it doesn’t merge now.

@Jiansen
Copy link
Contributor Author

Jiansen commented Aug 2, 2016

@gaearon , I rebased as suggested.

Do you think we should use the same error handling mechanism in release.sh as well, or run e2e at the beginning, or assume project will always properly tested before a project admin runs release.sh?

@ghost ghost added the CLA Signed label Aug 2, 2016
@Jiansen
Copy link
Contributor Author

Jiansen commented Aug 20, 2016

Rebased.

@gaearon , do you plan to add this PR back to milestone 0.3.0, if not merging it soon?

Currently, minor changes to config/paths.js will cause code conflicts due reordering of configs. I don't mind to rebase again when it is a good time to deliver. Just curious about the plan.

@ghost ghost added the CLA Signed label Aug 20, 2016
@Jiansen Jiansen force-pushed the clean-publish-eject branch 2 times, most recently from 9f5226d to ab49abe Compare August 20, 2016 18:45
@ghost ghost added the CLA Signed label Aug 20, 2016
@gaearon
Copy link
Contributor

gaearon commented Sep 2, 2016

Let’s get this in and maybe iterate on it later.
Thanks.

@gaearon gaearon merged commit 6c8713b into facebook:master Sep 2, 2016
@gaearon gaearon added this to the 0.4.0 milestone Sep 2, 2016
@gaearon
Copy link
Contributor

gaearon commented Sep 2, 2016

screen shot 2016-09-02 at 16 55 56

This is really nice. 👍

If you’d like, please feel free to make a followup PR to make eject result more natural.

For example I think it might make sense to try to write babel.dev.js and babel.prod.js without all those crazy require.resolves and then put the require.resolve mapping behind an // @remove-on-eject.

@Jiansen Jiansen deleted the clean-publish-eject branch September 2, 2016 21:03
@Jiansen
Copy link
Contributor Author

Jiansen commented Sep 2, 2016

@gaearon , good point. I will try as you suggested.

@Jiansen Jiansen mentioned this pull request Sep 2, 2016
stayradiated pushed a commit to stayradiated/create-react-app that referenced this pull request Sep 7, 2016
feiqitian pushed a commit to feiqitian/create-react-app that referenced this pull request Oct 25, 2016
@lock lock bot locked and limited conversation to collaborators Jan 22, 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