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

feat(gatsby): support ?. and ?? in generated pages #20036

Merged
merged 2 commits into from Dec 10, 2019
Merged

Conversation

pvdz
Copy link
Contributor

@pvdz pvdz commented Dec 10, 2019

After this change you should be able to use the new ?. and ?? operators at runtime (so in the resulting site). They will be transformed by Babel.

Basically tested with an empty repo in dev mode, added console.log(SEO?.foo ?? 'bar') before and after this change. Before it would crash on the new syntax and afterwards it would work. Both develop and build.

@pvdz pvdz requested a review from a team as a code owner December 10, 2019 12:47
Basically tested with an empty repo in dev mode, added `console.log(SEO?.foo ?? 'bar')` before and after this change. Before it would crash on the new syntax and afterwards it would work. Both `develop` and `build`.
@pieh pieh self-assigned this Dec 10, 2019
@pvdz pvdz requested a review from a team as a code owner December 10, 2019 16:02
Copy link
Contributor

@pieh pieh left a comment

Choose a reason for hiding this comment

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

Looking good. Updated readme to mention newly added plugins.

Not a blocker - do you think this warrants some e2e tests using new syntaxes to verify it works and catch potential regressions in future? This could be different PR.

I did manually verify that things work as expected, so this is good to go in as-is IMO

@sidharthachatterjee sidharthachatterjee added the bot: merge on green Gatsbot will merge these PRs automatically when all tests passes label Dec 10, 2019
@gatsbybot gatsbybot merged commit ea6185c into master Dec 10, 2019
@delete-merged-branch delete-merged-branch bot deleted the questions-runtime branch December 10, 2019 16:58
@pvdz
Copy link
Contributor Author

pvdz commented Dec 10, 2019

do you think this warrants some e2e tests using new syntaxes

I considered it. I've been in this position before (with many many more babel plugins) and in that case I had a bunch of those tests. But the e2e were so expensive relative to the syntax never really changing that those tests were discarded after some time. So in this case I kind of decided that checking whether they worked would suffice.

At some point these syntax features will end up in e2e tests organically and they'll be picked up. (We can synthetically speed that process up as a side effect without creating explicit new tests for it, I guess ;) )

@pvdz
Copy link
Contributor Author

pvdz commented Dec 10, 2019

Put differently, it would basically be testing whether Babel works.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: merge on green Gatsbot will merge these PRs automatically when all tests passes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants