Refactor whole codebase #75
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Gatsby forces you to use GraphQL for pre-processing data, and is generally inflexible. There's an overview here: https://medium.com/@tannerlinsley/%EF%B8%8F-introducing-react-static-a-progressive-static-site-framework-for-react-3470d2a51ebc that covers the main reasons why I prefer React-Static, but it basically boils down to: I didn't want to deal with GraphQL and custom plugins to pre-process the documentation data so the docs page doesn't need to ship >4 MB of JS, including a Markdown parser, two copies of the data (in HTML and JSON), doesn't need to parse that JSON multiple times between search-and-replacing the domain name, etc. This commit only deals with moving to React-Static; the docs page is still dog-slow, but the next commits will improve it considerably.
Future versions have a bug that prevents building the static site on Windows. React-Static v8 fixes the issue, but is not publicly available yet. react-static/react-static#1360
This commit touches almost every file in the codebase. There should be a lot of speed improvements, especially on the Documentation page. A lot less code is being shipped to the browser, and the random bugs on the Documentation table of contents should be gone. Visually, it's almost identical to the previous version.
|
Thanks, @cmmartti! Tomorrow I'll take a look at the code to see all the improvements! I just checked the demo on Netlify and it seems to work flawlessly |
|
I just noticed that I forgot to add Google Analytics back in. Is this something we use at all? |
|
Yes, we need it. It should be the UA: |
|
In the |
|
We need also to instruct CircleCI to use the files in the |
|
@cmmartti, I cannot build the project. I'm running on Node 12 on Win10. |
|
Also this didn't help: https://www.npmjs.com/package/@babel/plugin-proposal-nullish-coalescing-operator |
|
That's because The solution is to remove that use of |
|
There are also other https://github.com/cmmartti/pokeapi.co/blob/master/src/utils/useActiveAnchor.js#L34 |
|
I also had to run |
|
In the Maybe you can add one. I don't know if it's better to do it in |
|
It would be nice also to have a |
|
Other than that it works pretty well 😃 |
|
Code that is not in The favicon and manifest are added at build-time only, so you won't see them in dev mode. Can you elaborate on why you needed to add that package? I don't recall using it in the code. Maybe a dependency uses it, but npm should have installed it. |
|
Hi @cmmartti, below you find the whole log that was generated by It seems that the |
|
When I run which is about |
|
Hi @cmmartti, I would really like to merge this branch, I really think we need |
It's not used directly (react-static uses it), but in some situations it npm doesn't install it.
|
@Naramsim I removed use of the nullish coalescing operator (??) and added the |
|
Works perfectly on my machine, now the last thing is to modify the Then I would like to merge you branch to the |
|
I also noticed that it happens also on your Netlify: https://musing-mcclintock-d866d7.netlify.app/ |
|
Yeah, not entirely sure why that's happening. I've never seen it before. But I'm not at my computer right now so it will have to wait a bit. |
|
Yeah, no problem @cmmartti. It's not important :) |
|
I got the culprit, it's the GA library. I deployed a version without |
|
We can just strip it out then, and add in Google Analytics manually. Feel free to go ahead and do it if you wish, as I won't have time to work on it for a couple of days. |
Merged
|
Hi @cmmartti, in the end I think the issue is way broader. It's not a problem withing the GA package, but a problem running If I check the output of Note the I also saw that I tried to overcome the issue with I honestly don't know how to resolve this issue and I cannot find anything online that could help. PS: When on CircleCI and running |
Naramsim
added a commit
that referenced
this pull request
May 21, 2020
|
In the end I moved the execution of the build process from docker containers to the machine executor inside CircleCI. Now the CPUs are correctly detected and the :) I should add back GA |
|
Sorry I wasn't able to figure it out. I just haven't had time these last few days to think about it. React-Static isn't a very popular static site generator, and even the creator (Tanner Linsley, of React-Table and React-Query, among others) has abandoned it in favour of Next.js. However, I didn't find this out until after I'd already almost finished migrating the website to it. React-Static is still maintained by a company that uses it, and there's a new major version planned, but there's not a lot of people using it or working on it. I probably wouldn't use it again because of bugs like the ones we've run into, even though I like it a lot. Fortunately, it should be super easy to switch to another SSG in future if we need to now that the code has been cleaned up. Next.js recently got proper static site support, and I probably would have used that instead if I'd known it was coming. |
HRKings
pushed a commit
to HRKings/pokeapi.co
that referenced
this pull request
Jan 15, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a messy PR, but it's basically the result of me messing around with the code and trying to fix everything all at once. Preview at https://musing-mcclintock-d866d7.netlify.app/
A quick summary below:
Switch to React-Static from Gatsby:
Gatsby forces you to use GraphQL for pre-processing data, and is generally inflexible. There's an overview here that covers the main reasons why I prefer React-Static, but it basically boils down to: I didn't want to deal with GraphQL and custom plugins to pre-process the documentation data so the docs page doesn't need to ship >4 MB of JS, including a Markdown parser, two copies of the data in HTML and JSON, doesn't need to parse that JSON multiple times between search-and-replacing the domain name, etc.
Add alert notice about missing Sword+Shield data:
Refactor everything:
This commit touches almost every file in the codebase. There should be a lot of speed improvements, especially on the Documentation page. A lot less code is being shipped to the browser, and the random bugs on the Documentation table of contents should be gone. Visually, it's almost identical to the previous version.