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

hot-reload/incremental build JSON language files #43

Merged
merged 4 commits into from
Feb 5, 2021

Conversation

jngbng
Copy link
Contributor

@jngbng jngbng commented Jan 26, 2021

breaking change:
path option is deleted and users should load language JSON files using
gatsby-source-filesystem then load it using GraphQL query.

fix #4

breaking change:
`path` option is deleted and users should load language JSON files using
gatsby-source-filesystem then load it using GraphQL query.
@jdtzmn
Copy link

jdtzmn commented Jan 26, 2021

@jngbng can we display a warning that the path config is deprecated in the onCreateNode handler? Also, wouldn't a breaking change warrant incrementing the major version, as the semantic versioning guidelines suggest? Otherwise, users might unintentionally update to the breaking change.

I'd also like to thank you once again for your work on this. It is much appreciated!

@jngbng
Copy link
Contributor Author

jngbng commented Jan 27, 2021

@jdtzmn Thanks for good suggestion. The maintainer will pick the version when releasing to NPM as this project manages version using release-it tool. He/she should edit README.md in my PR, though.

Displaying a warning is a good idea! But onCreateNode handler seems not a good place to print a warning as it is called frequently and will cause excessive warnings. (That may be what we want, thought.) Since I am new to Gatsby plugin api, I have no idea of the best place. Another option would be post install hook of this package. (like core-js) Any suggestions are welcomed.

@jdtzmn
Copy link

jdtzmn commented Jan 27, 2021

@jngbng Makes sense. I was reviewing the PR and I saw the section in the README.md and figured I should mention it.

I found the following issue that addresses where to display deprecation warnings:
gatsbyjs/gatsby#6974

They recommend using the onPreBootstrap handler.

@jngbng
Copy link
Contributor Author

jngbng commented Jan 28, 2021

@jdtzmn Thanks for your suggestion. Now plugin prints an error message when deprecated "path" option still exists in plugin option.

image

@jdtzmn
Copy link

jdtzmn commented Jan 28, 2021

@jngbng Love it! Looks great. I think it's ready for @jimmyn to review.

@jimmyn
Copy link
Member

jimmyn commented Jan 28, 2021

I will check it asap, thanks for your contribution

@jimmyn
Copy link
Member

jimmyn commented Jan 28, 2021

If we are moving back to graphql we should also support loading namespaces separately. This is actually what I wanted to implement next and I need it myself. If it is a big page with lots of translation it does not make sense to load those translations on other pages but at the same time, you can have common translations for header, for example.

Is it going to work with a query like this:

query($language: String!) {
    locales: allLocale(filter: {lng: {eq: $language}, ns: {regex: "/(common|about)/"}}) {
      edges {
        node {
          ns
          data
          lng
        }
      }
    }
  }

Ideally, we should simplify it somehow or at least document

Another thing is that the language variable is inconsistent across the code
We now have language, lang, lng. I get that lng is shorter but I'd prefer consistency

@jngbng
Copy link
Contributor Author

jngbng commented Jan 31, 2021

@jimmyn Yes, it works as you mentioned. I added sections for it on README.md. c40fcb1

Also changed lang, lng to language.

@jimmyn jimmyn merged commit 3785097 into microapps:master Feb 5, 2021
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.

No rebuild when locales are changed
3 participants