Compiler fails on fresh build when using npm

colbyfayock commented 3 years ago

When doing a fresh build using npm instead of yarn, the build fails due to the lack of public/wp-search.json

It seems like npm resolves the files a bit differently and it's not finding it when it hits it

colbyfayock commented 3 years ago

some context here - i have ot use npm on the WP Engine Atlas deployment as it doesn't currently support yarn

im wondering if it makes sense just to move the file to a client-side request. i feel like it's been discussed before but kept it as an import

would help reduce the delivered site for bigger sites but of course wouldn't be as speedy

since search is global though, if we load it in the hook immediately when the page loads, it should still appear like it's just as fast as it should load before anyone can try to search something

any thoughts?

GuilleAngulo commented 3 years ago

Couldnt reproduce the error using npm ... I will try again tomorrow just for curiosity

I think that makes sense, once the file is created initializating Fuse on useEffect could work. Just wondering if there is any kind of delay on really big files, could worth making a test?

colbyfayock commented 3 years ago

@GuilleAngulo did you both remove wp-search.json and use npm build?

colbyfayock commented 3 years ago

definitely think its worth making a test. ill put something together to see what itl ooks like

GuilleAngulo commented 3 years ago

Yeah, removed all plugins and installed via npm... maybe i'm missing something

colbyfayock commented 3 years ago

yeah so weird and now all of a sudden im getting the same thing on netlify https://app.netlify.com/sites/next-wordpress-starter/deploys/60b28ac834d35a000760acbf not sure if you can see that

ill put together a POC, wont take long

GuilleAngulo commented 3 years ago

I'm seeing the log...weird.. I kept trying to reproduce, deleting .next, node_modules, wp-search.json and fresh install with npm but im getting no errors... I would say that it seems like something related to jsconfig.json, like is not getting that compiler options where the public folder is resolved... theoretically there is no need of an extra webpack configuration..

colbyfayock commented 3 years ago

thats so strange... not sure. i put together a clientside solution though: https://github.com/colbyfayock/next-wordpress-starter/pull/181

curious if there's a bug beyond that though if you're thinking you see something

colbyfayock commented 3 years ago

@GuilleAngulo im going to merge this in simply because i want to avoid the builds failing, but i'd love to get your feedback on it so i can retroactively make any tweaks if needed

GuilleAngulo commented 3 years ago

hey, sure. I will take a look at it (if not today, will try this week)

GuilleAngulo commented 3 years ago

Hey @colbyfayock , finally I got to take a look, sorry for the delay. Here are some thoughs:

Maybe it would be a good idea to move the SearchProvider from _app.js to inside the Layout component, wrapping the Nav. As this context is only used here for the search, could be a good idea to reduce the scope only targeting this part of the tree. It would be a little improvement, but as I understand it will avoid re-render everything inside _app when the provider state changes loading the data, and only the Nav will be re-rendered.
I was testing useSearchState(), and I mocked the function getSearchData to have a delay. As expected if the user types before the data loads no results are shown. Probably this would happen in very few cases but I think that can be a good idea to protect that behaviour to happen. Instead of having the current state variable at useSearchState, we could use two states: loading (boolean) and error (string). This way the hook can make this states available to the component, and we could set a disabled={loading} at the input while the data is being loaded (in most cases the user will never see that disabled). And maybe we can even separate that input into a component to style the loading and error states.
Finally another idea with other approach that ocurred to me while researching for the other PR: Another way we could do it is to read this file at _app getInitialProps and make it available at useSiteContext, having it available ahead of mounting. Not sure if it can have downsides and if it will remain having problems with the file path...

I would like to know your opinion on these thoughts, hope they are helpful =)

colbyfayock commented 3 years ago

Rerenders

From my non-scientific poking around, i think we're safe with rerenders

I put a bunch of console logs on the tree of the homepage:

My understanding is that only things that consume it, such as the Navigation, which uses useSearch and ultimately the context, will rerender.

Also https://reactjs.org/docs/context.html#caveats

Thoughts?

Search State

Great idea. Wanna check this out? https://github.com/colbyfayock/next-wordpress-starter/pull/190

getInitialProps

I'd be down to move this back into being read, the build issue is my biggest concern, im not sure if that would still help because my impression was node just wasn't resolving it between the build/compile. you weren't ever able to repro the main issue right?

GuilleAngulo commented 3 years ago

Re-renders: my mistake, you are absolutely right. I usually try to reduce the context at its minimum scope possible, so I ended up thinking it somehow can negatively affect the rest of the tree, but it only affects consumers 👍

Search: I will check the PR and write you there directly 💪

getInitialProps: Yeah I tried but I couldn't reproduce it ... I'll try to search a little more about it, if I can find something. But also the way is now seems totally fine to me, at client-side

colbyfayock / next-wordpress-starter