odysee-frontend icon indicating copy to clipboard operation
odysee-frontend copied to clipboard

Webpack upgrade

Open david0178418 opened this issue 2 years ago • 31 comments

Fixes

Opening as a draft as I still need to do some deeper testing to ensure there are no quirks. But the basic funcitonality works as expected.

I'd expect any issues to surface at build time as opposed to runtime. But just being a user, I'm not familiar with all the nooks and crannies, so any extra eyes for any potential oddities would be good.

Upgrade webpack and related changes as an incremental step toward implementing typescript, as mentioned here https://github.com/OdyseeTeam/odysee-frontend/issues/1178.

Rolls in https://github.com/OdyseeTeam/odysee-frontend/pull/1901

Other information

Webpack and related updates were made. Not all of the packages could be brought to current. Specifically, postcss and cssnano. postcss-rtl is no longer maintained and will not function with the latest major version of postcss.

PR Checklist

Toggle...

What kind of change does this PR introduce?

  • [ ] Bugfix
  • [ ] Feature
  • [ ] Code style update (formatting)
  • [x] Refactoring (no functional changes)
  • [ ] Documentation changes
  • [x] Other - Please describe: build upgrade

Please check all that apply to this PR using "x":

  • [x] I have checked that this PR is not a duplicate of an existing PR (open, closed or merged)
  • [TODO] I have checked that this PR does not introduce a breaking change
  • [ ] This PR introduces breaking changes and I have provided a detailed explanation below

david0178418 avatar Jul 23 '22 16:07 david0178418

I've been poking at this over the past couple days, and everything I've seen is working. Let me know if any of you run into any issues.

david0178418 avatar Jul 26 '22 23:07 david0178418

Might want to hold off changes to see what Tom thinks -- is the timing right?

Of course. No rush from my end. Take it at your leisure or even use this as a baseline reference down the line if this is something that needs to be done much later.

david0178418 avatar Jul 27 '22 12:07 david0178418

Of course. No rush from my end. Take it at your leisure or even use this as a baseline reference down the line if this is something that needs to be done much later.

Cool. I think this is good for the path to typescript, though. Let's wait for his feedback on timing and testing bandwidth.

infinite-persistence avatar Jul 27 '22 13:07 infinite-persistence

The changes regarding the images have been implemented. There is still one hanging issue with NODE_ENV being lost that I still need to resolve. My first guess is that it has something to do with some interaction between the webpack v4 and cross-env, but I'll get that worked out in my next pass (side note/FYI: cross-env is now in maintenance mode).

david0178418 avatar Aug 02 '22 01:08 david0178418

Inadvertently closed the PR. Reopened.

david0178418 avatar Aug 02 '22 01:08 david0178418

I wouldn't mind getting this in, but could use some testing on one of our test instances. Let's get it building first?

failing on Travis: https://github.com/OdyseeTeam/odysee-frontend/runs/7623226763?check_suite_focus=true

thanks for the effort @david0178418 !

tzarebczan avatar Aug 04 '22 19:08 tzarebczan

@tzarebczan Anytime!

Yeah, in the next batch of free time I have, I'll figure out why NODE_ENV isn't propagating in the CI environment. I'd previously set it in the script, but had forgotten that this was the same script ultimately used for dev as well. I'm guessing it has something to do with some interaction between webpack and cross-env.

Also, it might require some trial and error commits since I can't seem to reproduce the issue locally. My apologies to you and the team if it spams you all with borked build messages while I feel that out.

david0178418 avatar Aug 04 '22 19:08 david0178418

@tzarebczan if you give me a heads-up before you plan on putting this on a testing environment, I can do another package update and rebase against master to make it as updated as possible before testing.

david0178418 avatar Aug 12 '22 01:08 david0178418

I can put it up today, so let me know when it's ready!

tzarebczan avatar Aug 12 '22 13:08 tzarebczan

@tzarebczan Ok, you should be good to go now.

david0178418 avatar Aug 12 '22 14:08 david0178418

Got this during build: warning " > [email protected]" has incorrect peer dependency "webpack@^1 || ^2 || ^3 || ^4". [webpack-cli] Error: Unknown option '--display' [webpack-cli] Run 'webpack --help' to see available commands and options error Command failed with exit code 2.

tzarebczan avatar Aug 12 '22 14:08 tzarebczan

That's coming from our build script - maybe just remove? not supported anymore? image

tzarebczan avatar Aug 12 '22 14:08 tzarebczan

Thanks. I'll see about taking a look at it later this evening.

david0178418 avatar Aug 12 '22 15:08 david0178418

I shipped it on stashu.odysee.com without that. Seems to work properly, no other warnings. We'll give it a whirl over next few days to make sure nothing major creeps up.

tzarebczan avatar Aug 12 '22 17:08 tzarebczan

Sounds like a plan. Thanks guys.

david0178418 avatar Aug 13 '22 04:08 david0178418

So far things are looking good on the test site. Any things you can think of that would be problematic or to look out for, or basically since it builds, runs, should be fairly safe?

tzarebczan avatar Aug 15 '22 19:08 tzarebczan

Yeah, basically that. Since it's at the build level, failures should be catastrophic and up front.

The main things that I'd look to potentially break would be things like async imported, broken asset references and such. Most of the core functionality seems to touch on these, and they seem to be working as expected.

Aside from any other concerns others may have, the only other thing I'd want to double check on is that workflow yml change. I see that there's some potential discussion to be had around this since you created a ticket, so I want to be sure that this tweak is good to go for the time being.

david0178418 avatar Aug 16 '22 00:08 david0178418

@keikari noticed this weird checkbox thing, image

tzarebczan avatar Aug 16 '22 03:08 tzarebczan

Odd. Good catch.

I took a brief look at the selectors, and I'm seeing this in prod:

image

vs this in the new build:

image

It might be a couple of days before I can get to it, but I'll dig into it and see what's resulting in the difference.

david0178418 avatar Aug 16 '22 15:08 david0178418

  • It works fine on development build, so I'm thinking the production optimizer or new post-css thing yanked out the needed duplicate line.
  • But not sure why we needed to re-define the left color for it to work correctly in the first place.

image

infinite-persistence avatar Aug 17 '22 05:08 infinite-persistence

@infinite-persistence Thanks for pointing that out. Yeah, running the production build locally reproduces it, so this gives me a target to see what's up.

Btw, quick question. Does Odysee currently support right-to-left languages? I'm only asking because postcss-rtl is no longer maintained and is not compatible with the latest version of postcss. Not looking to try and entangle a change on that front with this change, but also a good time to trim any libraries that might happen to be unused and within arms reach.

david0178418 avatar Aug 19 '22 00:08 david0178418

Yup, we do support those languages. There is also an open bug for it (just fyi)

https://github.com/OdyseeTeam/odysee-frontend/issues/1229

infinite-persistence avatar Aug 19 '22 00:08 infinite-persistence

Just addressed the checkbox issue. Also, the branch has been freshly rebased against master. So worth poking at it again.

david0178418 avatar Aug 19 '22 01:08 david0178418

Thank you! rebuilding on stashu now...

tzarebczan avatar Aug 20 '22 18:08 tzarebczan

@infinite-persistence , I saw some sentry related changes that conflicted here and got things resolved on this side. All looks good here, but might want to double check to make sure it looks right to you as well.

david0178418 avatar Aug 26 '22 03:08 david0178418

Thanks for the merge and heads-up. Tried a full build with sentry auth tokens and all -- seems to be working fine.

  • On an unrelated note (just thinking out loud):
    • Seems like the vendor* stuff have been replaced with simple hashes

      • -->
    • But dev:web-server produces them, with weird "node_modules" prefix. Would be nice if dev/prod differences can be minimized, but probably not a big deal

      • image
    • In dev:web, I wonder where's the generated output now ... used to be in dist like production build, so we can have quick peeks. All in-memory now?

infinite-persistence avatar Aug 26 '22 05:08 infinite-persistence

Sorry for making you rebase/etc. I can try to ship this on Monday if it's all clean.

tzarebczan avatar Sep 23 '22 16:09 tzarebczan

@tzarebczan No worries at all. Considering how low level it is, it's a given that this should be gone over with a fine-tooth comb. And as I mentioned to @infinite-persistence in a previous comment, it would have been totally cool for you guys to decline this PR and just use it as a reference for later work. Whatever works for your timeline works for me.

david0178418 avatar Sep 25 '22 23:09 david0178418

@tzarebczan just wanted to double check here to make sure there are no other todos you'd like on this one.

david0178418 avatar Oct 06 '22 03:10 david0178418

All good from our end, just wanted to get the creator memberships stuff shipped this week before giving this a final whirl + prod deployment. I can take care of the conflicts then.

tzarebczan avatar Oct 07 '22 19:10 tzarebczan