Upgraded to Babel 7 #501
Upgraded to Babel 7 #501
Conversation
Codecov Report
@@ Coverage Diff @@
## master #501 +/- ##
======================================
Coverage 84.1% 84.1%
======================================
Files 171 171
Lines 5387 5387
Branches 1 1
======================================
Hits 4531 4531
Misses 856 856
Continue to review full report at Codecov.
|
|
netlify build is live at https://deploy-preview-501--bemuse.netlify.com/ |
|
Error is |
|
@thakkaryash94 yep, just reran on CircleCI, still the same |
|
I reset HEAD to old commit |
|
Hmm, that's odd. I have a currently-active PR to upgrade all the dependencies outside of Babel. Would you like me to get that done and merged first so you could rebase off of it? |
|
Sure, we can try. I locally merge |
|
@thakkaryash94 Have you tried running the in-browser tests and see if they work?
|
|
Ah, Scintillator. 😅 I knew it's gonna be a problem at some point... I'll ask our other maintainer @dtinth to have a look. |
|
Hi, sorry for not following on this PR. I was busy with lots of stuff. I am digging down to find the root cause of the problem. The test case causes Chrome Renderer Process to crash with this message logged into the console: This is really bizarre... I am figuring out why right now. |
|
I tried to step through the code line-by-line, and I reached a point where it crashed. The crashThe crash lies in this function, inside ParticleContainer.prototype.onChildrenChange = function onChildrenChange(smallestChildIndex) {
var bufferIndex = Math.floor(smallestChildIndex / this._batchSize);
while (this._bufferUpdateIDs.length < bufferIndex) {
this._bufferUpdateIDs.push(0);
}
this._bufferUpdateIDs[bufferIndex] = ++this._updateID;
};When I tried to log, The reason of crashI look at what could have caused
How to fix the crashChange let batch = new PIXI.particles.ParticleContainer(undefined, {
position: true,
alpha: true
}) |
…g a crash
I tried to step through the code line-by-line, and I reached a point where it crashed.
The crash
---------
The crash lies in this function, inside `node_modules/pixi.js/lib/particles/ParticleContainer.js`.
```js
ParticleContainer.prototype.onChildrenChange = function onChildrenChange(smallestChildIndex) {
var bufferIndex = Math.floor(smallestChildIndex / this._batchSize);
while (this._bufferUpdateIDs.length < bufferIndex) {
this._bufferUpdateIDs.push(0);
}
this._bufferUpdateIDs[bufferIndex] = ++this._updateID;
};
```
When I tried to log, `bufferIndex` is `NaN`, causing an infinite `while` loop where `this._bufferUpdateIDs` is getting pushed with `0` until Chrome crashes due to out-of-memory error.
The reason of the crash
-----------------------
I look at what could have caused `bufferIndex` to become `NaN`, and I found out that...
1. We created a ParticleContainer with `maxSize` of `null`. This should trigger the default of 1500
https://github.com/bemusic/bemuse/blob/f822a33371c2d97e12d1ef3af16db272b6444be4/src/scintillator/nodes/object.js#L77-L80
2. However, the commit pixijs/pixijs@22c5248 that gets released in Pixi v4.5.0 contained this change:
```diff
-class ParticleContainer extends core.Container {
+export default class ParticleContainer extends core.Container
-
- constructor(maxSize, properties, batchSize)
+ constructor(maxSize = 1500, properties, batchSize = 16384)
{
super();
- batchSize = batchSize || 15000; //CONST.SPRITE_BATCH_SIZE; // 2000 is a nice balance between mobile / desktop
- maxSize = maxSize || 15000;
```
Instead of using `||` to default `maxSize` to 1500 when it is falsy, it has been changed to use ES6 default parameters **which only works with `undefined`.** This means `null` value will no longer set `maxSize` to a default of 1500. This caused `bufferIndex` to subsequently become NaN, causing an infinite loop.
3. Our `yarn.lock` in master pins Pixi.js to v4.1.0:
```
pixi.js@^4.1.0:
version "4.1.0"
```
but in this PR, it has been upgraded to v4.8.2, which contains the change that caused the crash:
```
pixi.js@^4.1.0:
version "4.8.2"
```
Has the `yarn.lock` been removed and entirely rebuilt from scratch in the process?
How to fix the crash
--------------------
Change `null` to `undefined`.
```js
let batch = new PIXI.particles.ParticleContainer(undefined, {
position: true,
alpha: true
})
```
|
I pushed a fix to your branch — 075cc4b. |
This is due to the fact that CircleCI Environment Variables are not made available to forked PR builds, since these environment variables contain sensitive information. That means for forked PRs, DANGER_GITHUB_API_TOKEN will be unset, causing Danger to crash. However, we can't just expose the environment variables for forked PR builds, as that would essentially make the secret keys public. To resolve this completely, we need to figure out a way to allow Danger to make GitHub comments WITHOUT putting the GitHub Personal Access Token of akibot in the public, but that is an issue for another time. For now, what we can do is just disable Danger for forked PR.
|
Pushed another fix that makes the |
|
I was adding and removing babel deps when migrating, so removed yarn.lock completely and regenerated, my mistake. I think it will better if start using fix package versions instead of |
|
@thakkaryash94 I agree that could prevent the issue in the future. However I think it’s quite strange as non-major version bumps shouldn’t cause the app to crash, and upgrading Babel shouldn’t force Pixi to bump version. Another possible fix is to revert I verified this by running: And looking at the But I think, for this PR, there is no need to revert |
closes #499 . Upgraded tested dev and prod build.