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): Add eslint rules to warn against bad patterns in pageTemplates (for Fast Refresh) #28689

Merged
merged 25 commits into from
Jan 15, 2021

Conversation

LekoArts
Copy link
Contributor

@LekoArts LekoArts commented Dec 18, 2020

Description

This adds new ESLint rules to our default configuration that enforce two things in page templates (so src/pages/* and pages created with createPage):

  • No anonymous default exports
  • Page templates must only export one default export (the page) and query as a named export

Additionally we also re-enabled the pascalCase rule (as it doesn't seem to trip up <Styled.p> from theme-ui anymore.

This all happens for Fast Refresh as it requires these code patterns to work correctly and without a full hard reload.

This PR will also include #28911


Old comment about Babel Plugin This adds babel plugin to `develop` stage when fast-refresh is enabled that checks wether page template is hot reloadable with fast-refresh: - page template (default export) need to be named component (and it need to start with uppercase actually) - page template must not have additional exports (other than page query)

Messages in Console:

image

image

image

TODO:

  • make warning messages more actionable and in general worded better ( current ones are for most part just "placeholders" to make sure this works )
  • print warnings also in browser console (this is already handled)
  • find actually proper place to make those checks - babel plugin is/was good starting point because we get the AST, but there are problems with it:
    • this is in fact lint rule and not transformation (unless we make it automatic transformation, but that is risky, plus it's not always possible - like multiple exports can't be transformed into something working with fast refresh). So this is really better suited to be run with eslint loader ... but users can disable builtin eslint configuration very easily and use custom one with gatsby-plugin-eslint. We don't "merge" eslint options and don't have requiredRules, defaultRules kind of deal in same way we have for babel
    • because this is babel plugin - it will reuse cached results, meaning if we have page template resulting in warning, do edit -> save -> rollback to previous file content - it will reuse already cached thing without reprinting the warning. We could throw errors/warnings in the plugin but that would completely fail compilation and I'm not sure if this is very user friendly. Babel plugins doesn't seem to have builtin error/warning tracking (not sure here - I just couldn't find anything about this)
    • we could create webpack loader and use webpack's warning mechanism, but this might mean we would need to parse code from string (into AST) again - but this should work around the caching problems - and also actually work well when it comes to showing the warning both in terminal and in browser console

Related Issues

[ch19773]
[ch22489]
[ch22491]

@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Dec 18, 2020
@LekoArts LekoArts added topic: hot reloading* and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Dec 18, 2020
@LekoArts LekoArts changed the title tmp: add preliminary babel plugin for page templates to warn when those won't work with fast refresh feat(gatsby): Add eslint rules to warn against bad patterns in pageTemplates (for Fast Refresh) Jan 7, 2021
pvdz
pvdz previously approved these changes Jan 11, 2021
Copy link
Contributor

@pvdz pvdz left a comment

Choose a reason for hiding this comment

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

I think it's fine to go? Please do go through the comments. Most things are improvements rather than problems. Some things are only problems if you say they are :)

LekoArts and others added 2 commits January 12, 2021 10:29
…t config (#28911)

* feat(gatsby): add required eslint rules even if user has custom eslint config

* handle case when extends is single string, add some tests

* some extra existance checking

* more exact checks for existance of eslint-loader rule

* let's see if slash fixes win32 + env var for rules

* another try

* don't use slash after all

Co-authored-by: LekoArts <lekoarts@gmail.com>
@LekoArts LekoArts marked this pull request as ready for review January 13, 2021 16:32
pvdz
pvdz previously approved these changes Jan 14, 2021
Copy link
Contributor

@pvdz pvdz left a comment

Choose a reason for hiding this comment

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

Two cases are nits so I'd say merge with or without them applied.

packages/gatsby/src/utils/webpack-utils.ts Outdated Show resolved Hide resolved
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