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

Add inject to "bundle everything" list #2871

Merged
merged 5 commits into from
Oct 26, 2016
Merged

Add inject to "bundle everything" list #2871

merged 5 commits into from
Oct 26, 2016

Conversation

jacogr
Copy link
Contributor

@jacogr jacogr commented Oct 25, 2016

Getting the following issues while trying to replicate (https://github.com/ethcore/parity/issues/2868 - at this point getting a diefferent issue, not the one listed, but it still doesn't work) -

external_"vendor_lib":1 Uncaught ReferenceError: vendor_lib is not defined(anonymous function) @ external_"vendor_lib":11 @ inject.js:1t @ inject.js:1(anonymous function) @ module.js_from_dll-reference_vendor_lib:1181 @ inject.js:5t @ inject.js:1(anonymous function) @ utf8.js:244138 @ inject.js:4t @ inject.js:1(anonymous function) @ utils.js:3935 @ inject.js:3t @ inject.js:1(anonymous function) @ requestmanager.js:281200 @ inject.js:13t @ inject.js:1(anonymous function) @ web3.js:281186 @ inject.js:11t @ inject.js:1(anonymous function) @ index.js:11175 @ inject.js:11t @ inject.js:1(anonymous function) @ inject.js:17(anonymous function) @ inject.js:35836 @ inject.js:10t @ inject.js:1(anonymous function) @ multi_inject:10 @ inject.js:1t @ inject.js:1n.0 @ inject.js:1(anonymous function) @ inject.js:1
194app.js:4 Uncaught TypeError: Cannot read property 'eth' of undefined

Obviously inject.js (injecting web3) like parity.js should not need the vendor stuff. (I have seen this internally as well.)

@ngotchac Could you take a look to make sure my approach here is right from a build prespective?

Fixes #2872

@jacogr jacogr added A0-pleasereview 🤓 Pull request needs code review. M5-ui labels Oct 25, 2016
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 86.082% when pulling 7c7a36c on jg-fix-vendor_lib into 135d5d0 on master.

@ngotchac
Copy link
Contributor

ngotchac commented Oct 26, 2016

Ok so I removed the Webpack DLL (used in dev) in production builds, this should do the trick.
I also updated the environment index file so it uses the PARITY_URL if provided, so you can test that it works:

  1. Run PARITY_URL="127.0.0.1:3000" NODE_ENV="production" npm run build
  2. Execute node build-server
  3. Go to http://localhost:3000/dev.web3.html and check web3 in the JS Console

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.04%) to 86.074% when pulling 55e5d74 on jg-fix-vendor_lib into 135d5d0 on master.

@@ -41,7 +41,6 @@ module.exports = {
'signaturereg': ['./dapps/signaturereg.js'],
'tokenreg': ['./dapps/tokenreg.js'],
// library
'inject': ['./inject.js'],
'parity': ['./parity.js'],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Remove parity.js as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is useful for the Webpack CommonChunks usage, so it separates parity.js changes from the others (better perf. I 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.

ok, cool.

name: 'commons'
})
);
plugins.push(
new webpack.optimize.CommonsChunkPlugin({
chunks: [ 'parity' ],
chunks: ['parity'],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As here - web3/inject & parity should be treated the same.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok that's true

@@ -1,4 +1,4 @@
{
{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These 2 spaces look suspicious :)

@@ -7,6 +7,6 @@
<title>dev::Web3</title>
</head>
<body>
<script src="inject.js"></script>
<script src="web3.js"></script>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can we make this /parity-utils/web.js (same with /parity-utils/parity.js in dev.html and for dapp.html as well.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Will do

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.001%) to 86.115% when pulling 9f4fab5 on jg-fix-vendor_lib into 135d5d0 on master.

@jacogr jacogr merged commit c8809b3 into master Oct 26, 2016
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) to 86.104% when pulling c9b9d2d on jg-fix-vendor_lib into 135d5d0 on master.

@jacogr jacogr added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Oct 26, 2016
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.004%) to 86.111% when pulling c9b9d2d on jg-fix-vendor_lib into 135d5d0 on master.

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.

3 participants