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

[ZEPPELIN-1850] Introduce Webpack (front) #1805

Closed
wants to merge 17 commits into from

Conversation

1ambda
Copy link
Member

@1ambda 1ambda commented Dec 25, 2016

What is this PR for?


1. The purpose of this PR is introducing webpack to zeppelin-web

  • It supports super-fast javascript compilation and (hot) reload
  • It helps to simplify, unify build process.
  • It enables for us to use import, export features.
  • It's more modern stack than grunt. This will encourage other developers to contribute (the most import thing IMO).

2. This PR is designed to improve build process gradually!!!

  • So, Angular module loading depends on the import sequence as we did before in index.html. (see index.js).
  • Also, CSS and HTML file is not managed by webpack including its live reloading

These will be handled by additional PRs.

3. This PR is not big


What type of PR is it?

[Improvement]

Todos

  • - Setup webpack.config.js
  • - Resolve global variable zeppelin problem
  • - Support webpack in karma (npm run test)
  • - Annotate the result bundle using webpack ng annotate plugin
  • - Fix eslint violations
  • - Livereload for HTML, CSS

What is the Jira issue?

ZEPPELIN-1850

How should this be tested?

  • cd zeppelin-web && rm -rf node_modules bower_components node
  • npm install
  • npm run test
  • npm run build
  • npm run dev and open localhost:9000: check live-reload works regarding to html, css, js files
  • cd .. && mvn clean package -pl 'zeppelin-web' -DskipTests && ./bin/zeppelin-daemon.sh restart and open localhost:8080

Screenshots (if appropriate)

N/A

Questions:

  • Does the licenses files need update? - NO
  • Is there breaking changes for older versions? - NO
  • Does this needs documentation? - NO

@tae-jun
Copy link
Contributor

tae-jun commented Dec 25, 2016

So coool!

I tested following your instructions and it worked like a charm 👍
And live-reloading time remarkably decreased.

LGTM!!!

"dev:server": "webpack-dev-server --hot",
"dev:watch": "grunt watch-webpack-dev",
"prestart": "grunt pre-webpack-dev",
"start": "npm-run-all --parallel dev:server dev:watch",
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably it's a nit; since there is default command npm start to run the script in npm, I think it will be better to use another for this. How about dev instead of start? :)

Copy link
Member Author

@1ambda 1ambda Dec 26, 2016

Choose a reason for hiding this comment

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

Thanks for review :) @AhyoungRyu

Since there is default command npm start ....
How about dev instead of start? :)

So, you mean start:server and start:watch. right?

Copy link
Contributor

Choose a reason for hiding this comment

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

I just meant we run dev server script \w npm run start for now, so I suggested npm run dev for this.

Copy link
Member Author

Choose a reason for hiding this comment

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

I see! Fixed in 61dd5e5

@1ambda 1ambda force-pushed the ZEPPELIN-1850/introduce-webpack branch 2 times, most recently from 702aaac to b5bfe9d Compare December 26, 2016 06:28
@AhyoungRyu
Copy link
Contributor

@1ambda To quickly test this branch, I tried to change some css components and html, js files. It's working really nicely(and super fast). Great work indeed! Seems it solved ZEPPELIN-1805 as well :)
If you don't mind can we update zeppelin-web/README.md?

@1ambda
Copy link
Member Author

1ambda commented Dec 27, 2016

@AhyoungRyu I didn't know about ZEPPELIN-1805 but good to hear that it is solved by this PR.

Also, docs was updated to include npm run dev command in 8442eba :)

@1ambda 1ambda force-pushed the ZEPPELIN-1850/introduce-webpack branch from 8442eba to e467b96 Compare December 27, 2016 09:58
@Leemoonsoo
Copy link
Member

@1ambda Great improvement!
Tested and it works well.

One thing is, previously console.log() was printed only in dev mode. After this change, console.log() is printed both dev and dist. If it is not very difficult, can console.log() be muted in non-dev mode?

Other than that, LGTM.

@1ambda 1ambda force-pushed the ZEPPELIN-1850/introduce-webpack branch from e467b96 to 6aa25b8 Compare December 28, 2016 02:05
@1ambda
Copy link
Member Author

1ambda commented Dec 28, 2016

@Leemoonsoo Thanks!

Remove console.log in dist

I missed it. Previously, grunt stripped all console.log using this config

uglify: {
...
        'drop_console': true
...

So i added strip-loader to achieve same behavior while building in 5e1696f

About Size

## before
-rw-r--r--  1 lambda  staff   113K Dec 28 11:33 scripts.1546a550da5ec483.js

## after
-rw-r--r--   1 lambda  staff   116K Dec 28 12:36 app.f4ebf1a368629b338f2d.js

@1ambda 1ambda force-pushed the ZEPPELIN-1850/introduce-webpack branch from 5e1696f to 920589b Compare December 28, 2016 04:42
@Leemoonsoo
Copy link
Member

Tested console.log in dev and dist. It now works as expected.

LGTM and merge to master if there're no further discussions.

@asfgit asfgit closed this in 3f28865 Dec 29, 2016
asfgit pushed a commit that referenced this pull request Jan 4, 2017
### What is this PR for?

Removed [strip-loader](https://issues.apache.org/jira/browse/ZEPPELIN-1850) which was added to remove `console.log` in production.
But we can achieve the same result by using uglify option. I missed it in #1805

- `strip-loader` has a bug as you can see in below (screenshot section)
- Deleting it help us to keep the package list simple
- Also using the same configuration for uglify both in webpack and grunt will ensure that vendor package have the consistent behavior when they are managed by webpack

Also, this should be merged before #1812 since we need to modify `yarn.lock` in #1812

### What type of PR is it?
[Bug Fix | Improvement]

### Todos

Done at once

### What is the Jira issue?

[ZEPPELIN-1850](https://issues.apache.org/jira/browse/ZEPPELIN-1850)

### How should this be tested?

1. Add this code to any js file in master branch and execute `npm run build` to lint. You will get an error

```
        console.log('editor isn\'t loaded yet, returning..');
```

2. Test the same command and code within this branch. It will work.

### Screenshots (if appropriate)

```
ERROR in ./src/app/notebook/paragraph/paragraph.controller.js
Module build failed: SyntaxError: Unexpected token, expected , (197:8)

  195 |             }
  196 |           }
> 197 |         });
      |         ^
  198 |       } else {
  199 |         BootstrapDialog.confirm({
  200 |           closable: true,

  ./src/index.js 53:0-59
Child html-webpack-plugin for "index.html":
        + 1 hidden modules
Running "useminPrepare:html" (useminPrepare) task
Configuration changed for concat, uglify, cssmin

Running "concurrent:dist" (concurrent) task

    Running "svgmin:dist" (svgmin) task
    Total saved: 0 B

    Done, without errors.

    Execution Time (2016-12-30 06:21:32 UTC)
    loading tasks  121ms  ▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇ 70%
    svgmin:dist     51ms  ▇▇▇▇▇▇▇▇▇▇▇▇▇▇ 30%
    Total 172ms

    Running "copy:styles" (copy) task
    Copied 17 files

    Done, without errors.

    Execution Time (2016-12-30 06:21:32 UTC)
    loading tasks  127ms  ▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇ 77%
    copy:styles     38ms  ▇▇▇▇▇▇▇▇▇▇▇ 23%
    Total 165ms

Running "postcss:dist" (postcss) task
>> 17 processed stylesheets created.
>> No "concat" targets found.
Warning: Task "concat" failed. Use --force to continue.

Aborted due to warnings.

Execution Time (2016-12-30 06:21:30 UTC)
loading tasks       117ms  ▇▇▇▇ 6%
useminPrepare:html   22ms  ▇ 1%
concurrent:dist      1.5s  ▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇ 80%
postcss:dist        252ms  ▇▇▇▇▇▇▇▇▇ 13%
Total 1.9s

npm ERR! Darwin 15.6.0
npm ERR! argv "/Users/lambda/.nvm/versions/node/v6.9.1/bin/node" "/Users/lambda/.nvm/versions/node/v6.9.1/bin/npm" "run" "build"
```

### Questions:
* Does the licenses files need update? - NO
* Is there breaking changes for older versions? - NO
* Does this needs documentation? - NO

Author: 1ambda <1amb4a@gmail.com>

Closes #1823 from 1ambda/ZEPPELIN-1850/remove-strip-loader-in-webpack and squashes the following commits:

137bcdd [1ambda] review: Fix invalid uglify plugin opt
3d3a581 [1ambda] fix: Remove strip-loader to avoid single quote error
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants