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

fix: runtime build issues #685

Merged
merged 3 commits into from
Jun 15, 2020
Merged

fix: runtime build issues #685

merged 3 commits into from
Jun 15, 2020

Conversation

mikemurray
Copy link
Member

Issue

Cannot run app due to regenerator runtime issues

Solution

Update babel config

Breaking changes

none

Testing

  1. App should run and be usable

Signed-off-by: Mike Murray <[email protected]>
Copy link
Member

@willopez willopez left a comment

Choose a reason for hiding this comment

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

LGTM

@janus-reith
Copy link
Collaborator

@mikemurray The babel config I made obsolete for the new version was added back in #682, introducing this error.
Instead of adjusting the babel config, I'd consider just removing it again.

I don't see us using specific features that would make it reasonable to introduce potential upstream issues by diverging from the nextjs defaults here.

@willopez
Copy link
Member

willopez commented Jun 9, 2020

@janus-reith just saw your comment, after I approved it. I am open to removing it and simplifying the config. PR welcomed.

@janus-reith
Copy link
Collaborator

@willopez Yes, I would prefer if we discuss in #683 or some seperate issue instead how to approach the tests cleanly, and which should be there in general.( I think @HarisSpahija also wanted to contribute to that.)

The previous storefront version also had a bunch of babel dependencies I'd rather not have included here again.

TBH my preference would be to revert #682 and not merge this one, and fix the tests instead.
Maybe a short term solution could be to have that babel config in a separate test-specific folder? Or any other way to scope it to the tests only, if it actually is necessary.

@willopez
Copy link
Member

willopez commented Jun 9, 2020

Sounds good, lets discuss on #682

Signed-off-by: Mike Murray <[email protected]>
@willopez
Copy link
Member

This PR removes unnecessary babel config and some dependencies. A new PR will be created to remove snapshot test and related dependencies add react testing library and cypress. See for more context: #683 (comment)

@willopez willopez merged commit a8bc67c into trunk Jun 15, 2020
@willopez willopez deleted the fix-babel-runtime branch June 15, 2020 17:27
@kieckhafer kieckhafer mentioned this pull request Sep 25, 2020
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.

3 participants