Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Build embedded Parity JS properly and separatly #4426

Merged
merged 12 commits into from
Feb 9, 2017
Merged

Conversation

ngotchac
Copy link
Contributor

@ngotchac ngotchac commented Feb 3, 2017

For paritytech/parity-extension#19

Updates Webpack and build the embedded parity bar as a separate build.
Emits a embed.json file that contains the needed assets.

Some components of the ~/ui have been deleted from the index.js file : the editor and the PasswordStrength ones. This is because their dependencies are quite big, and Webpack 2 tree-shaking doesn't work that well : it wouldn't exclude them from the embed build, whereas the components are not used.

It seems that the production build was in fact not working (minification wasn't occurring) coming from the fact that we didn't transpile EthereumJS-Tx. Now it's fixed.
Webpack has been updated to the latest stable release.

@ngotchac ngotchac added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Feb 6, 2017
export ActionbarExport from './Actionbar/Export';
export ActionbarImport from './Actionbar/Import';
export ActionbarSearch from './Actionbar/Search';
export ActionbarSort from './Actionbar/Sort';
Copy link
Contributor

Choose a reason for hiding this comment

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

Aligning with the exports as done in Form & Container would actually be quire good -

export Actionbar, { Export as ActionbarExport, ...} from './Actionbar;

Alternatively since these are sub components, making them static on Actionbar, only exporting it and then using Actionbar.Export could also work.

export Label from './Label';
export RadioButtons from './RadioButtons';
export Select from './Select';
export TypedInput from './TypedInput';
Copy link
Contributor

Choose a reason for hiding this comment

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

+1

if (!process.env.EMBED) {
const worker = require('./worker');

module.exports = { setupWorker: worker.setupWorker };
Copy link
Contributor

Choose a reason for hiding this comment

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

Since nothing else is used, why not -

const setupWorker = require('./worker').setupWorker;

module.exports = { setupWorker }

Would probably add a TODO here for when babel finally supports non top-level imports

@jacogr
Copy link
Contributor

jacogr commented Feb 6, 2017

Build is failing, seems like polyfill issues maybe?

@jacogr jacogr added A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. and removed A0-pleasereview 🤓 Pull request needs code review. labels Feb 6, 2017
@ngotchac
Copy link
Contributor Author

ngotchac commented Feb 6, 2017

Updated : tests passing

@ngotchac ngotchac added A0-pleasereview 🤓 Pull request needs code review. and removed A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. labels Feb 6, 2017
@jacogr
Copy link
Contributor

jacogr commented Feb 7, 2017

Looks good, however only to be merged after 1.5.2 is released. (Makes backporting much more difficult)

@jacogr jacogr added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Feb 7, 2017
@gavofyork gavofyork added the A1-onice 🌨 Pull request is reviewed well, but should not yet be merged. label Feb 7, 2017
@jacogr jacogr removed the A1-onice 🌨 Pull request is reviewed well, but should not yet be merged. label Feb 9, 2017
@jacogr jacogr merged commit e8175f4 into master Feb 9, 2017
@jacogr jacogr deleted the ng-webpack-embed branch February 9, 2017 14:31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants