Skip to content

Added production ready settings for react theme #2587

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Mar 8, 2018
Merged

Added production ready settings for react theme #2587

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ca75c2e
Added production ready settings for react theme
thangngoc89 Mar 7, 2018
565559c
Update package.json
chenglou Mar 7, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions jscomp/bsb/templates/react/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,21 @@
# ${bsb:name}

Run this project:
## Run this project:

```
npm install
npm start
# in another tab
npm run webpack
```

After you see the webpack compilation succeed (the `npm run webpack` step), open up the nested html files in `src/*` (**no server needed!**). Then modify whichever file in `src` and refresh the page to see the changes.

**For more elaborate ReasonReact examples**, please see https://github.com/reasonml-community/reason-react-example

## Build this project

```
npm run build
npm run webpack:build
```

2 changes: 1 addition & 1 deletion jscomp/bsb/templates/react/bsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
"subdirs" : true
},
"package-specs": [{
"module": "commonjs",
"module": "es6",
Copy link
Contributor

Choose a reason for hiding this comment

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

Using es6 by default is probably a bad idea, as it'll decrease compatibility with various tools. I think you've already had issues using es6 modules with bs-jest? commonjs on the other hand is the lowest-common denominator. As far I understand, the primary benefit of using es6 is to be able to show off tiny hello world bundles, but I don't think that is justification enough for increasing friction for newcomers.

Copy link
Member

Choose a reason for hiding this comment

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

@thangngoc89 what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@glennsl I've successfully config bs-jest for working with es6, this is my current config:

"jest": {
    "transformIgnorePatterns": [
      "/node_modules\\/(?!bs-jest|bs-platform|reason-react|bs-react-test-renderer)/"
    ],
    "testMatch": [
      "**/__tests__/**/*_test.bs.js"
    ]
  }

This is nothing crazy, I'm telling jest to transpile bs-jest, bs-platform, reason-react, bs-react-test-renderer and friends because jest doesn't transpile anything in node_modules by default. Perhaps you would accept a PR to bs-jest's README about this @glennsl ?

the primary benefit of using es6 is to be able to show off tiny hello world bundles

Hmm, the main benefit is definitely not for the hello world, It's a good default for treeshaking and a small bundle in real world app. The ecosystem has adapted with ES6 modules and I would say it's sane to enable it right now.

Copy link
Contributor Author

@thangngoc89 thangngoc89 Mar 8, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

A sidenote: You could set transformIgnorePatterns to [ ""] but that would introduced a lot of overhead.

Copy link
Member

Choose a reason for hiding this comment

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

Are we sure that es6 also works well with other tools than bs-jest?

Copy link
Contributor

Choose a reason for hiding this comment

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

Let me throw in my 2c. ES6 is the standard in frontend dev. If we want to be taken seriously as a replacement for Babel and friends, we need to show we care about bundle size.

Copy link
Contributor

@glennsl glennsl Mar 8, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Perhaps you would accept a PR to bs-jest's README about this @glennsl ?

Definitely, but I don't think this is actually all the required configuration. Don't you need to add babel-jest too?

Hmm, the main benefit is definitely not for the hello world, It's a good default for treeshaking and a small bundle in real world app. The ecosystem has adapted with ES6 modules and I would say it's sane to enable it right now.

The few who require tiny bundle sizes can probably figure out how to enable es6 modules through some guide. I don't think it's a good idea to (possibly) sacrifice newcomer experience for the very minor convenience of having an advanced optimization enabled by default. At the very least I'd like to see this tested for a bit to have a better idea of its consequences before enabling it as the default.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense. We can test it in master and release it (or not) based on that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Definitely, but I don't think this is actually all the required configuration. Don't you need to add babel-jest too?

Yep. I do. I think it's best that we change this back to commonjs. Also I want to see this happens #2418 . This won't require a custom config for jest

Copy link
Member

@chenglou chenglou Mar 9, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Ok, let's use commonjs and put a comment right above on using es6 (bsconfig.json parser accepts comments and trailing commas).

Edit: or maybe put it in webpack.config.js or README? @thangngoc89?

"in-source": true
}],
"suffix": ".bs.js",
Expand Down
12 changes: 7 additions & 5 deletions jscomp/bsb/templates/react/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,20 +6,22 @@
"start": "bsb -make-world -w",
"clean": "bsb -clean-world",
"test": "echo \"Error: no test specified\" && exit 1",
"webpack": "webpack -w"
"webpack": "webpack -w",
"webpack:build": "NODE_ENV=production webpack"
},
"keywords": [
"BuckleScript"
],
"author": "",
"license": "MIT",
"dependencies": {
"react": "^15.4.2",
"react-dom": "^15.4.2",
"reason-react": ">=0.3.0"
"react": "^16.2.0",
"react-dom": "^16.2.0",
"reason-react": ">=0.3.4"
},
"devDependencies": {
"bs-platform": "^${bsb:bs-version}",
"webpack": "^3.8.1"
"webpack": "^4.0.1",
"webpack-cli": "^2.0.10"
}
}
3 changes: 3 additions & 0 deletions jscomp/bsb/templates/react/webpack.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,11 @@
const path = require('path');
const outputDir = path.join(__dirname, "build/");

const isProd = process.env.NODE_ENV === 'production';

module.exports = {
entry: './src/Index.bs.js',
mode: isProd ? 'production' : 'development',
output: {
path: outputDir,
publicPath: outputDir,
Expand Down