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] Remove strip-loader (webpack) #1823

Conversation

1ambda
Copy link
Member

@1ambda 1ambda commented Dec 30, 2016

What is this PR for?

Removed strip-loader 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

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..');
  1. 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

@khalidhuseynov
Copy link
Contributor

Thanks for fix! I faced this error after merging master into #1775 and applied temporary fix by deleting \'. It seems to be problem with strip-loader. tested and LGTM!

@Leemoonsoo
Copy link
Member

Tested this branch, but i can see log message printed in javascript console (both in dist and dev mode). Is it expected behavior?

@1ambda
Copy link
Member Author

1ambda commented Jan 2, 2017

Opps! I used invalid opt layout :( Fixed in 828c086
Thanks! @Leemoonsoo

screenshot

screen shot 2017-01-02 at 3 26 09 pm

@1ambda 1ambda force-pushed the ZEPPELIN-1850/remove-strip-loader-in-webpack branch from 828c086 to 137bcdd Compare January 2, 2017 22:22
@Leemoonsoo
Copy link
Member

Tested and LGTM!

Merge to master if there're no more comments.

@asfgit asfgit closed this in 49423eb Jan 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants