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

OEUI-193 : Upgraded webpack to version 3.8.1 #137

Merged
merged 1 commit into from
Aug 7, 2018

Conversation

rhenshaw56
Copy link
Contributor

@rhenshaw56 rhenshaw56 commented Aug 7, 2018

JIRA TICKET NAME
OEUI-193: Update Webpack and babel-loader to latest version.

SUMMARY
This Pull Request upgrades webpack from version 1.15.0 to 3.8.1. I originally planned to use version 4.16.5 but ran into into compatibility issues, notably with plugins and loaders such as:

  1. html-webpack-plugin was screaming errors, the fix was to use a fork of it which some nice folks apparently created which I don't recommend here. See threads:
    DeprecationWarning: Tapable.plugin is deprecated. Use new API on `.hooks` instead
    Can't load plugins using webpack 4

  2. I tried using both extract-text-plugin and mini-css-extract-plugins it was building perfectly but styles were not being loaded in, couldn't get it to work. checkout issue.

  3. The reason I didn't upgrade the babel-loader version was because it's actually Ideal and stable for the 3.8.1 version of webpack I used and webpack 4 requires babel-loader versions >= 7 plus we already got that in openmrs/react-components.

@coveralls
Copy link

coveralls commented Aug 7, 2018

Pull Request Test Coverage Report for Build 911

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 93.839%

Totals Coverage Status
Change from base Build 908: 0.0%
Covered Lines: 935
Relevant Lines: 950

💛 - Coveralls

Copy link
Contributor

@Efosaok Efosaok left a comment

Choose a reason for hiding this comment

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

LGTM


const nodeModulesDir = path.resolve(__dirname, '../node_modules');
const env = process.env.NODE_ENV;
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to specify Our NODE_ENV version for this in the env file or does it use what ever NODE_ENV is on the machine?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there is no env file, variables are set in npm scripts so you get access to them at runtime

@@ -114,25 +143,22 @@ if (env === 'production') {
archive.directory(`${outputPath}`, '');

archive.finalize();
}))

} else if (env === 'deploy') {
Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed deployed was deleted is it delibrate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes it was! if you checked the npm scripts, you'd notice that the former made use of deploy as an option passed, we can use development and production instead

}

plugins.push(new BrowserSyncPlugin({
proxy: {
target: browserSyncTarget
}
},
// this ain't seem to be working, apparently would require webpack-dev-server;
Copy link
Contributor

Choose a reason for hiding this comment

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

If it is not working can we remove it, we should avoid having non-functional code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jomadoye it's not non functional, thing is it reloads normally even by default check here but cos we're bootstrapping the app with the standalone, I believe previous builds get cached on the standalone hence the manual hard reloading and cache clearing so this is for making sure the OEUI actually reloads on changes. what I had in mind was to use webpack-dev-server here to force browser-sync to hard reload (thereby setting this to false) but it was losing the context of the standalone server, so I had to get rid of it, that's what I meant in those comments

package.json Outdated
"build:dev": "npm run clean && webpack --progress --colors --mode=dev --target=web",
"build:prod": "npm run test && npm run build",
"build:deploy": "webpack --progress --colors --mode=deploy --target=web",
"watch": "webpack --progress --colors --watch --mode=deploy --target=web",
"watch": "NODE_ENV=\"development\" webpack --progress --colors --watch --cache=false",
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @rhenshaw56
I see you changed the --mode=deploy flag to "NODE_ENV=\"development\" in the watch script and also updated the webpack config with that. But I noticed that this change is yet to be reflected in the build:deploy script, I am not sure the build:deploy script would work as it is unless you also reflect the change as you did with the watch script, let me know if I am wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@IAMOTZ so the previous webpack version had the --mode option and deploy was just a value to pass around and use in the config to know what mode you were actually running ie. production, dev etc, version 4 also has the mode option but 3 doesn't.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it works as it should normally

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 should prolly update the other script commands also seeing as they have deploy on them

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, I just tried it out and it is not working. This is a snapshot of the error:

screen shot 2018-08-07 at 11 00 50 am
Since you updated this part of the configuration:

else if (env === 'deploy') {
	outputFile = `${outputFile}.js`;
	vendorOutputFile = "vendor.bundle.js";
	outputPath = `${config.LOCAL_OWA_FOLDER}${config.LOCAL_OWA_FOLDER.slice(-1) != '/' ? '/' : ''}${THIS_APP_ID}`;
	devtool = 'source-map';

}

to this:

if (env === 'development') {
	outputFile = `${outputFile}.js`;
	vendorOutputFile = "vendor.bundle.js";
	outputPath = `${config.LOCAL_OWA_FOLDER}${config.LOCAL_OWA_FOLDER.slice(-1) != '/' ? '/' : ''}${THIS_APP_ID}`;
	devtool = 'eval-source-map';
}

The build:deploy script is supposed to build the project and inject it in the OpenMRS standalone but with the changes you are making in the webpack.config.js, the build:deploy script needs to be also updated to something like this:

"build:deploy": "NODE_ENV=\"development\" webpack --progress --colors --target=web",

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch mahn! updating scripts asap

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@IAMOTZ done

Copy link
Contributor

@jomadoye jomadoye left a comment

Choose a reason for hiding this comment

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

LGTM

fixed bug with output path
Copy link
Contributor

@IAMOTZ IAMOTZ left a comment

Choose a reason for hiding this comment

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

Looks Good

@mogoodrich mogoodrich merged commit b598e2a into openmrs:master Aug 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants