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

Use fake firehose client in react-storybook build #14072

Merged
merged 1 commit into from Mar 30, 2017

Conversation

islemaster
Copy link
Contributor

@islemaster islemaster commented Mar 29, 2017

image

Replaces firehose imports with a fake firehose client in the react-storybook build. This is a speculative fix for the storybook deploy failing on Circle, per Will's observation that the failures began with the addition of the AWS SDK in #14053.

This might not be a bad idea, even if it doesn't fix the Circle build on its own - we really shouldn't have storybook hitting AWS in any way.

@ewjordan
Copy link
Contributor

Should we be concerned about the AWS SDK adding memory weight in the browser, as well?

If that's the case, we could switch over to the AWS Lambda version of this with a bit of work - we have functioning code there which imports Jquery instead of the AWS SDK, the unsolved bit is hooking it all up in Cloudformation.

@islemaster
Copy link
Contributor Author

It would be interesting to check on whether Asher's PR changed the size of our bundles significantly, but I'd be surprised if it did - it wasn't the first change to add the aws sdk to our project, so we're already shipping that weight somewhere. In the case of this error, we're running out of memory during our build step, not at runtime, so I'm guessing it's a fairly independent issue.

@islemaster
Copy link
Contributor Author

Update: This change didn't deploy successfully, we still had the out of memory error. It looks like we're also building source maps, which is not something we really need in the deployed version. I'll try and figure out how to turn that off.

@islemaster
Copy link
Contributor Author

Wow... that didn't work either. In fact, it failed in the exact same place - creating chunk assets - and still has SourceMapGenerator_serializeMappings in the reported security context. What is going on?

We shoudn't be blocking staging on a failed storybook deploy, so I'm going to disable the storybook deploy altogether for now and troubleshoot tomorrow.

@islemaster
Copy link
Contributor Author

I've reduced this PR to its first commit, which stubs firehose in storybook builds. This apparently resolved an issue for @Erin007 today, so I'd like to merge just this change, even though it doesn't resolve the Circle storybook deploy. Following up on a different branch with further work on re-enabling storybook deploy.

image

@ashercodeorg
Copy link
Contributor

LGTM. Just so that I'm clear, the issue isn't that test is trying to send Firehose API calls (it shouldn't be, the method returns early on local and test environments). The issue is the memory overhead of loading the AWS SDK?

@islemaster
Copy link
Contributor Author

I'm not even sure it's memory overhead at this point - I can't see all of the error @Erin007 posted from storybook, but it looks more like an initialization problem caused just by importing the sdk in an unusual context. But I do know we're never going to need that sdk in storybook and you guys have wrapped it nicely (thank you for that!) so we might as well stub it to remove any chance of it being the issue. 🤷‍♂️

Copy link
Contributor

@Erin007 Erin007 left a comment

Choose a reason for hiding this comment

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

The error was basically that those node modules didn't exist or couldn't be found.

@islemaster
Copy link
Contributor Author

Hmm... that might have been solved with just a yarn call to install missing dependencies.

@islemaster islemaster merged commit 12e1667 into staging Mar 30, 2017
@islemaster islemaster deleted the fake-firehose-in-storybook branch March 30, 2017 18:09
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.

None yet

4 participants