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

Add Webpack watch and dev server scripts #97

Merged
merged 8 commits into from
Feb 13, 2021

Conversation

simonmarklar
Copy link

@simonmarklar simonmarklar commented Dec 14, 2020

  • added yarn serve/ yarn start command using webpack dev server
  • added yarn watch for those using their own web server
  • added fixpack to keep package.json uniform

Please note that while i've tested that it builds and launches/displays in a browser im still having issues with jellyfin detecting my chromecast so cant fully test :'(

@YouKnowBlom
Copy link
Contributor

Is the webpack config large enough to warrant 4 files

@simonmarklar
Copy link
Author

Is the webpack config large enough to warrant 4 files

each file has a different purpose and keeps the options for each purpose in one place. I find it easier to grok this way but if you prefer it to be in one file with command line args i can do that :)

@simonmarklar
Copy link
Author

@YouKnowBlom i've merged the configs down to one. Webpack 5 has a bunch of new stuff which makes this really neat :) also figured i'd change the config over to typescript seeing as the rest of the codebase is headed that way

@YouKnowBlom
Copy link
Contributor

Awesome, I hope I didn't discourage you :p. Looks very neat now! Will take an extra look when I get home 👍

It's a busy time of the year so excuse me for taking a while to reply.

@simonmarklar
Copy link
Author

all good mate - I've not looked at webpack 5 before this ticket so I just went with what I am familiar with, reading the docs really helped on one of my insomnia nights ;)

all good on reply speed - this isnt our jobs ;)

@ferferga
Copy link
Member

ferferga commented Jan 2, 2021

@simonmarklar Can you rebase this please? Last PR that converts everything to TS ^^.

tsconfig.json Outdated Show resolved Hide resolved
@simonmarklar
Copy link
Author

@simonmarklar Can you rebase this please? Last PR that converts everything to TS ^^.

OK that should be all done.

I'm not 100% sold on the webpack config being typescript, there was a few hoops to jump through. What's peoples thoughts? Its nice having the typings for config but it comes at a cost of several dev dependencies

Copy link
Contributor

@hawken93 hawken93 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

plus my comment in jellyfinActions.ts, but I think I'm going to do a refactoring to async syntax later.

Would like changes for the two other comments if you agree with them.

tsconfig-webpack.json Show resolved Hide resolved
.eslintrc.js Show resolved Hide resolved
-
- removed eslint config that is automatically added by
  eslint-pligin-prettier
@simonmarklar
Copy link
Author

@hawken93

plus my comment in jellyfinActions.ts, but I think I'm going to do a refactoring to async syntax later.

i dont want to refactor the code in jellfinActions.ts in this PR - I only put the undefined in there to appease eslint and make it so I could run builds and verify that things still worked.

There's a ton of refactoring that needs to be done to bring the code in line but IMHO we're better off doing focused PRs - plus i see so much in progress work and I know you're smashing things out so im willing to bet its already been done by someone :)

Would like changes for the two other comments if you agree with them.

I dont think they are necessary - see my comments on them for more info :)

Copy link
Member

@ferferga ferferga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for taking so long in getting this merged. Thank you very much for your contribution!

@ferferga ferferga requested a review from hawken93 February 12, 2021 23:45
Copy link
Contributor

@hawken93 hawken93 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, I have gotten my questions answered. Would probably have preferred the promise resolve() to not be altered at all to make sure the linter still notes it as a problem. But that's very minor, especially with additional checks coming in that will cause us to rewrite away from promises 99% of cases

@ferferga ferferga merged commit c7c67de into jellyfin:master Feb 13, 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.

4 participants