-
Notifications
You must be signed in to change notification settings - Fork 6
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
[dependencies] Update to React 16, Webpack 4, Babel 7 #88
[dependencies] Update to React 16, Webpack 4, Babel 7 #88
Conversation
Note that bundle.js isn't actually deleted, just dramatically changed, I've heard that Github UI has trouble with displaying large diffs. |
@hydrosquall thanks for for upgrading the dependencies! I've been meaning to do this for a while. curious, could you factor out the changes from #86, so that this only includes the dependency upgrades? |
Yes, of course! I should have done that up front, I only mixed them up bc I
didn’t anticipate there being any hurdles with the parcel/netlify proxy
(but was too optimistic).
…On Wed, May 15, 2019 at 3:48 AM Micah Stubbs ***@***.***> wrote:
@hydrosquall <https://github.com/hydrosquall> thanks for for upgrading
the dependencies! I've been meaning to do this for a while.
curious, could you factor out the changes from #86
<#86>, so that this
only includes the dependency upgrades?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#88?email_source=notifications&email_token=ACE2MM3K6GHBOGPYVVMI4O3PVO54VA5CNFSM4HMJBDQKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODVNZ7EI#issuecomment-492543889>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ACE2MMY5J3NEFME2DMLJM3DPVO54VANCNFSM4HMJBDQA>
.
|
db7fc90
to
95148b8
Compare
@micahstubbs I pulled out the changes from #86 since that would otherwise have modified package.json, and regenerated the lockfile / bundle.js. |
nice! will have a look |
doing some troubleshooting
probably need to run |
reading more about this error https://stackoverflow.com/questions/29894154/chrome-neterr-incomplete-chunked-encoding-error |
interesting, so Firefox gives us a more useful error message:
|
made two small commits to preserve the webpack progress bar feature (looks like it got moved to a plugin sometime between weppack 1 and webpack 4) |
Thanks for bringing back the color and progress bar functionality :)! Unfortunately I haven't been able to reproduce the chunked encoding error, but I've gotten it in other projects before if I ran Are you still seeing the incomplete chunked encoding message? |
I tried out this modified branch together with the docker-compose setup over in enjalot/blockbuilder#232 and it seems to work :)! |
huh, will take another look. I'd like to ensure that the local blockbuilder + local blockbuilder-search UI dev setup still works, as a baseline. |
Ok, so I am trying this:
|
@hydrosquall what does your environment look like? here's mine:
|
I’ve tried this on both my work and home laptops, on the work laptop I’m
using the same node and yarn versions as you but on my home laptop I’m
using node 12.
```bash
node -v
v12.1.0
yarn -v
1.16.0
```
Do you run into the chunk issues with `yarn build` too, or just with
buildWatch? I just tried this again locally using the docker-compose setup linking this local copy to the `blockbuilder` with buildWatch and buildWatch in chrome, and wasn't able to get the chunked encoding issue.
…On Mon, May 20, 2019 at 11:41 PM Micah Stubbs ***@***.***> wrote:
@hydrosquall <https://github.com/hydrosquall> what does your environment
look like? here's mine:
node -v
v11.10.0
yarn -v
1.16.0
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#88?email_source=notifications&email_token=ACE2MM43TB6JZ6P46MLPKTTPWNVNJA5CNFSM4HMJBDQKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODV2VCDY#issuecomment-494227727>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ACE2MM4NDS3NHYCUYN43PE3PWNVNJANCNFSM4HMJBDQA>
.
|
If we end up hitting a standstill in reproducing the chunking issue, what I could try instead is to do a version of #90 without the library upgrades. This would let us test the search page by itself without needing to spin up What do you think? (To simplify things, I've put together the PR I described above here: #91 ). If we merge that first, it might help with testing on this branch. |
@hydrosquall I fixed the chunk encoding problem 🎉 the solution was:
thanks for your patience with this one. It's important to me to make sure that we keep the current blockbuilder + blockbuilder search setup working, since this is how we run the site in production right now. of course, I'm open to improving how we run the site in production also, but later. one change at a time 😄 |
so, once I get a review and enjalot/blockbuilder#235 goes in, we should be able to merge this one too. |
Woohoo!! Thanks for diving into this so deeply, I’m so happy to see that
this is working.
Maintaining compatibility with the existing production setup makes a lot of
sense to me. We may find a project like nodenv relevant in the future to
ensure people run the expected version of node together with the
application (https://github.com/nodenv/nodenv )
…On Wed, May 22, 2019 at 2:03 AM Micah Stubbs ***@***.***> wrote:
@hydrosquall <https://github.com/hydrosquall> I fixed the chunk encoding
problem 🎉
the solution was:
- use node v12.3.0
- use yarn to install packages for blockbuilder the editor
- some handlebars config changes in PR enjalot/blockbuilder#235
<enjalot/blockbuilder#235>
thanks for your patience with this one. It's important to me to make sure
that we keep the current blockbuilder + blockbuilder search working, since
this is how we run the site in production currently.
of course, I'm open to improving how we run the site in production also,
but later. one change at a time 😄
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#88?email_source=notifications&email_token=ACE2MM2HMCZ5QYMGAFWB3ZTPWTO4VA5CNFSM4HMJBDQKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODV57WQI#issuecomment-494664513>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ACE2MMZY5RAOEN2KB35PWRTPWTO4VANCNFSM4HMJBDQA>
.
|
thanks for doing this and digging in. I'll let @micahstubbs hit the merge button when ready |
merged! thanks for the contribution @hydrosquall and the review @enjalot 📈 |
This PR sits on top of changes from #85, but I can drop those commits if that PR doesn't end up getting merged first. It addresses #87 by bringing this project up to the latest react so that people can use current UI libraries to add to the project.
Primary Changes
.babelrc
config file so that parcel and the webpack file can share the configuration.class
syntax because React.createClass is no longer used in react 16, using this jscodeshiftThe preview deploy branch for this set of changes is available here, I believe everything builds correctly.
https://5cd74c6f696d0e00070244d8--blockbuilder-search-hydrosquall.netlify.com/
I also confirmed locally that the package maintains functionality when used with the core
blockbuilder
app usingnpm link
.