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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃嵄 Bento Storybook: Define all BUILD_CONSTANTS #35560

Merged
merged 3 commits into from
Aug 6, 2021

Conversation

caroqliu
Copy link
Contributor

@caroqliu caroqliu commented Aug 6, 2021

Otherwise we get IS_PROD is not defined and similar - example.

@caroqliu caroqliu requested a review from samouri August 6, 2021 17:54
@amp-owners-bot
Copy link

amp-owners-bot bot commented Aug 6, 2021

Hey @alanorozco! These files were changed:

build-system/tasks/storybook/preact-env/webpack.config.js

Copy link
Member

@samouri samouri left a comment

Choose a reason for hiding this comment

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

Do you also need to update main/build-system/tasks/storybook/amp-env/webpack.config.js

@@ -67,14 +67,12 @@ export const timelines = () => {
const timelineUserId = '3450662892';
return (
<Twitter
bootstrap="https://3p.ampproject.net/2104170104001/vendor/twitter.js"
Copy link
Member

Choose a reason for hiding this comment

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

Why is this part of the PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was being passed in because INTERNAL_RUNTIME_VERSION wasn't previously available.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually... I'm going to remove this separately, since there's a few like this sprinkled around and they may involve other changes.

@caroqliu
Copy link
Contributor Author

caroqliu commented Aug 6, 2021

Do you also need to update main/build-system/tasks/storybook/amp-env/webpack.config.js

The build constants are presumably already defined in AMP mode, since the same code path doesn't yield the same errors, so I thought it would be extraneous to include it here. Would you recommend updating it anyway?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Bento
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants