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

Removed redundant webpack configuration #55

Merged
merged 3 commits into from
Nov 5, 2018

Conversation

DannyT
Copy link
Contributor

@DannyT DannyT commented Nov 4, 2018

I've been poring over the webpack setup as a learning exercise (very valuable thanks). I'm a noob at this so this could well all be trash but figured I'd submit a PR in case it's of value.

I've removed what appeared to be redundant/old configuration. This makes the config a tiny bit smaller but perhaps a bit more digestible and understandable to folks new to it.

I've also added some comments and links where I learned what was going on.

@the-simian
Copy link
Member

the-simian commented Nov 4, 2018

I can see why some of these seem redundant, maybe I can help there

  • the comments are an aesthetic choice, but I left them there for future source-code divers. They need to stay. I might even add more comments, believe it or not so that the source is easier to understand. Unlike other programming styles, with self-documenting function names, a big webpack config isn't always easy to understand without picking it apart.

  • some of the paths variables are unused, but I included them so that they are easy to spot. They will be more important when I convert the project to a scaffold, and you'll just have to trust me that they are relevant

 'typeof WEBGL_RENDERER': JSON.stringify(true),	  'typeof WEBGL_RENDERER': JSON.stringify(true)
 'process.env.NODE_ENV': JSON.stringify('development')

The typeof WEBGL_RENDERER is important for browser compatibility, the node_env for platform, unless this happens to be no longer true. I'd rather not break edge cases, did you test this on every OS and browser?

  • line 19 can probably go away, I think. sure.

  • reload: false fixed bugs in my own build environment, I'm leaving that in.

  • I want to explicitly list the output paths even if they are defaults so that someone less familiar with webpack knows where to look without needing to go to the docs. I might change this when I make more documentation for the entire project

  • the node variables, we need to leave at least fs, because that affects some imports in other libs, even if it does nothing right now

I really really appreciate the PR, and I don't want to discourage future effort. I personally feel like comments and explicit destinations of builds/etc are more-digestible and not less digestible. The comments that were in create-react-app for instance really helped me understand their build process.

I guess there's some thought that will go into this when I convert this to a scaffold - which I know I have not done yet.

I want you to know, while I'm not going to pull this in - I am really thankful for the PR. I am thankful you are reading the source and spending time with it. I spent some time away from the project in September and October while my contract work took a surge in activity. I am just now picking back up my usual cadence.

I think the thing I should do next is spend some cycles curating issues and making atomic pieces of work that others can pick up. The weeks preceding my big art-asset update felt like major landscaping, and I need to spend some time writing about the Tiled integration, which is just now taking off.

I think after I do that, there will be really clear points of entry where you can jump in and contribute, which I'm looking forward to.

Big picture for this PR: I want the config to be well commented and actually be 'explicit' over conventional. This is mainly because webpack 4 did make it 'zero config' the previous 3 versions moved the furniture around a lot, and I want jsut about anyone to know that say dist is the output because "it says so" and not from webpack 4 tribal knowledge.

Big picture for the future: I am leaning harder and harder on Tiled integration, and making Tiled the 'defacto' editor for worlds and behavior. I just gave a talk on Nov 1st at Thunderplains about this and when the video is available I'll put it in the docs. It explains a lot of the 'why' of it. For instance at this exact moment, I'm trying to integrate decent raycast-style lighting by making it so that you can add properties to layers and entities in Tiled. I need to document these props and spend time talking about them with contributors as time moves on. This is a bit of a different direction than the 'jetpack' direction I was considering earlier (if you check the other issues). I was discussing that I think TIled will always be a superior tool to, say impact-weltmeister, which was a web implementation fo an editor for impact.js. I contributed to that some time ago (check my node migration of it) and after thinking about it, the time and effort to build such a thing again isn't worth it.

I know this was sort of a long message for a short PR. You showing interest means an absolute ton to me, and I'm taking it as maybe you taking interest in some of this discussion and strategy. Its really important to me you understand that not taking this PR does not mean I don't value your effort. I 100% value you and your time.I am honored you took the time to read the code and interact with me here. I see it as the start of a conversation about making this tool better, and I want to get you involved in ways you feel excited about.

Thank you very much. Let me curate these issues. I'll try to get this easier for contributors. I hadn't needed to do that beforehand, but I think it is valuable now.

@DannyT
Copy link
Contributor Author

DannyT commented Nov 4, 2018

This is all cool and no probs re not accepting the PR, I expected as much and the points about cross platform are noted, I just assumed they were legacy but makes sense to keep compatibility if there's any risk there (FWIW I'm running it in WSL on win10). However, I think there are a couple of points worth revisiting:

  • I didn't delete any comments, I added them :)
  • You have the WEBGL_RENDERER vars declared twice in the webpack config (with and without the typeof declaration), I just removed the ones without the typeof. They're also declared a third time in the index.html too.

Keep up the good work and thanks for the thorough explanation 👍

@the-simian
Copy link
Member

HA! you're totally right about the comments! I misread the diff!
as for the WEBGL_RENDERER - let me look into this. I want to make sure that truly redundant. I do see I have them with and without.

I tell you what - put some of the unused var delcarations back in, and the reload: false back in, and some of the explicit output paths, and lets reopen this and include your comments.

I'll just go ahead and pull this when you do

@the-simian the-simian reopened this Nov 4, 2018
@DannyT
Copy link
Contributor Author

DannyT commented Nov 5, 2018

@the-simian I believe I've reinstated those bits including the node config. But please do not feel under any obligation to include this still. It was much an exercise for my own learning and submitting it with subsequent dialogue answered a bunch of my "what's that for?" questions.

I see those node properties set in a lot in projects but it's pretty obtuse and other than "fixes some stuff in some environments" I've no idea what it's actually for?

@the-simian the-simian merged commit d76e1b9 into simiancraft:master Nov 5, 2018
@the-simian
Copy link
Member

the-simian commented Nov 5, 2018

I pulled it in, I'll check it out and make sure its still working fine, I'm in the middle of implementing a shadowcasting module though, and I'd like to see this through first

Assuming we're all set there, might be about time to publish to npm!

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.

None yet

2 participants