-
-
Notifications
You must be signed in to change notification settings - Fork 32.1k
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
"Maximum call stack size exceeded " fixes #4337 #4374
Conversation
@newoga Removing |
@nathanmarks @newoga It sounds like a babel issue to me 🤔 |
@nathanmarks No idea, when did this start happening? I haven't investigated yet but either way I was planning on removing this plugin for next release anyway. I'd probably vote to just do so before wasting too much time on it. I had opened an issue about removing it some time ago: #3948 |
@newoga Ok let's remove it. |
@newoga Ummm. So it looks like it was just pure coincidence. Now removing it isn't making a different 😄 |
@nathanmarks can't you use this patch until we can find a decent fix about it ? |
@mzgnr We could, but I want to see what else can be done about it |
@newoga BTW it's only happening if I run the whole |
@oliviertassinari We could merge this, I don't see a better option right now after playing with it myself. I think this may need to be posted on the babel phabricator. It's not the file as when the babeling is broken up into smaller parts it doesn't happen. |
Oh man! That's frustrating, 😆. I spent sometime installing various older versions of babel and babel related dependencies and didn't get far enough to eliminate the issue either. Looking at their change log though, it looks like babel-traverse has been going through a lot of changes. |
@newoga I have a branch anyways with that facebook implementation copied for the change to address that issue. I'll ask on babel slack too see if anyone knows. |
I'm wondering, that looks like a hacky and temporary workaround. I would rather see this fixed at the Babel level. What do you think about closing this PR and keeping the issue open? @nathanmarks Thanks for opening an issue on their tracker. |
I'm wondering... how you didn't face this issue ? I was not able to build package on v0.15-stable branch without this fix 🤔 |
@mzgnr My only guess is that a newer version of one of our dependencies changed the behavior and is causing this issue. I haven't been able to pinpoint when it happened though. |
@newoga Anyways. I was lucky enough to find the fix for this issue. This is the only fix i could find without changing project structure 😐 |
We'll use this if babel hasn't fixed the issue before our next release. I agree with @oliviertassinari |
I also agree with @oliviertassinari . https://phabricator.babeljs.io/T7402#78943 here there is an explanation for this issue and his argument is true. stack size parameter adds more horse power to node.js & v8 engine. I think its not mostly babeljs issue its caused by this https://github.com/callemall/material-ui/blob/0.15-stable/src/svg-icons/index.js file. I guess its generated from icon index generator. That is the main problem rather than babel. |
@mzgnr He instructed me to post the issue on phabricator, so he must believe it is something |
@@ -23,7 +23,7 @@ | |||
"scripts": { | |||
"build": "npm run build:icon-index && npm run build:babel && npm run build:copy-files", | |||
"build:icon-index": "babel-node ./scripts/icon-index-generator.js", | |||
"build:babel": "babel ./src --ignore *.spec.js --out-dir ./build", | |||
"build:babel": "node --stack-size=10000 ./node_modules/.bin/babel ./src --ignore *.spec.js --out-dir ./build", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure this works on Windows? Shouldnt it be more like node --stack-size=10000 ./node_modules/babel-cli/bin/babel.js ./src --ignore *.spec.js --out-dir ./build
?
Because .bin/babel is a linux file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@CumpsD I don't use Windows :( Can you test it if you are on Windows ? I can update PR with your suggestions :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Partly, at least it now processes svg-icons (on Windows) but in the end it errors out with:
npm ERR! [email protected] build:babel: `node --stack-size=10000 ./node_modules/babel-cli/bin/babel.js ./src --ignore *.spec.js --out-dir ./build`
npm ERR! Exit status 3221225725
npm ERR!
npm ERR! Failed at the [email protected] build:babel script 'node --stack-size=10000 ./node_modules/babel-cli/bin/babel.js ./src --ignore *.spec.js --out-dir ./build'.
Not sure yet what it means :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, Google tells me
Exit code 3221225477 is C0000005 or ACCESS_VIOLATION
No clue why though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will continue in the issue :) Have another suggestion
Would this be an option?
Splitting it into a build without icons and a separate one for the icons. This works on Windows :) |
@CumpsD would rather do the stack arg as the full build should work |
But it does not :( crashes on index.js in svg-icons |
Fixes " Maximum call stack size exceeded " #4337
I know this is a dirty fix. This might show people how to fix this issue. We also should update Readme about this issue. I'll send PR for Readme file.
Regards!