AsciiJakob / NoDistractions

Simple, bloat-free website blocker with a focus on intuitiveness
https://addons.mozilla.org/en-US/firefox/addon/nodistractions-website-blocker/
MIT License
3 stars 1 forks source link

Fix build issues with filename capitalization #5

Closed tuurep closed 2 months ago

tuurep commented 3 months ago

Hey, I was unable to build your extension due to some case sensitivity problems with filenames, causing errors like this:

$ npm install $ npm run build:firefox

ERROR in blocked
Module not found: Error: Can't resolve '/src/blocked/Blocked.js' in '/home/tuure/projects/NoDistractions'

ERROR in settings
Module not found: Error: Can't resolve '/src/settings/Settings.js' in '/home/tuure/projects/NoDistractions'

ERROR in unable to locate '/home/tuure/projects/NoDistractions/src/background/background.html' glob

Making the capitalization consistent allows me to build. I suspected it may be the case that on Windows the capitalization doesn't matter, but on Linux it makes a difference.

See this StackOverflow answer for example:

https://stackoverflow.com/a/45968534

tuurep commented 3 months ago

Ah, an awkward problem:

https://github.com/AsciiJakob/NoDistractions/blob/0c6a34f82cc91e24af82ceb88244ee5034aac50d/webpack.config.mjs#L25

...makes all the .js files lowercase anyways, and now I have broken references.

I'm considering making all files lowercase-kebab-case.xyz

tuurep commented 3 months ago

This was so much more complicated than I thought but now it all builds and works :smile:


kebab-case doesn't really fit because of things like:

BlockHandler.updateRequestListener();

Would look out of place as:

block-handler.updateRequestListener();

So instead I attempted to make webpack output the files in the format of:

dist/background/Background.js

To summarize my goals were (in priority):

  1. Be able to build the extension
  2. Have consistent style for filenames (because it's related to why it wouldn't build as well)
AsciiJakob commented 2 months ago

Hey, thank you for the interest and merge request! I apologize for taking so long before reviewing it, I completely missed it. I figured I would get an email about something like this, and I never check my Github notifications because it's usually just dependabot alerts LOL.

Thanks for making the build process work on Linux, I was oblivious to the capitalization problems as it's fine on Windows. Thank you for making the naming conventions more consistent, as well. There was a long break before when I first added everything and when I wrote the settingsuitilites thingy, so I guess I sort of forgot about the naming conventions and everything, I was probably quite lazy and confused in general - I just wanted to add a new feature 😄. Like you figured out the reason the background files have their first letter capitalized is because they are modules, but it's nice to just have all the files capitalized like that, just like you have done.

I'm gonna make sure it still builds properly on Windows, if it does i'll merge 👍

AsciiJakob commented 2 months ago

Everything works just like it should. Thank you for the contribution! I will now have a look at the issue you submitted.

tuurep commented 2 months ago

Thanks!

I figured I would get an email about something like this, and I never check my Github notifications because it's usually just dependabot alerts LOL.

Yeah this can happen easily :smile: btw I looked at Settings > Notifications and looks like what gets e-mailed can be customized pretty well and Dependabot has its own toggle. (I actually had dependabot e-mail spam in the past lol)

AsciiJakob commented 2 months ago

That's really useful, I think I have resolved it now. Cheers! I wrote quite a lengty reply to #4 just now, looking forward to hearing back!