ondras / rot.js

ROguelike Toolkit in JavaScript. Cool dungeon-related stuff, interactive manual, documentation, tests!
https://ondras.github.io/rot.js/hp/
BSD 3-Clause "New" or "Revised" License
2.33k stars 254 forks source link

reorganised imports for FOV module #153

Open mingos777 opened 5 years ago

mingos777 commented 5 years ago

Removed default imports for the FOV module in order to enable intellisense on exported classes.

Example valid code after the change:

import { PreciseShadowcasting } from "rot-js/lib/fov/precise-shadowcasting";
const fov: PreciseShadowcasting = new PreciseShadowcasting(lightPassesFunc);
ondras commented 5 years ago

Interesting! I have two comments:

1) can you explain why and how does this change fix the issue you were encountering? Completely eradicating export default from the whole codebase is not something I welcome with joy. When a source file provides exactly one class, a default export seems pretty straightforward to me.

2) is there a particular reason for removing file extensions from import statements? This change breaks the native module implementation in browsers that requires file extensions to be present (the result has to resolve to a valid URI, after all).

ondras commented 5 years ago
1. can you explain why and how does this change fix the issue you were encountering? Completely eradicating `export default` from the whole codebase is not something I welcome with joy. When a source file provides exactly one class, a default export seems pretty straightforward to me.

Ah, I missed your post in https://github.com/ondras/rot.js/issues/148#issuecomment-448410672. So this is already explained.

mingos777 commented 5 years ago
  1. As explained in the issue you linked, eliminating default exports and replacing them with named ones allows the IDE to provide intellisense (code completion) and automatic imports. More on why default exports are bad: https://basarat.gitbooks.io/typescript/docs/tips/defaultIsBad.html and https://blog.neufund.org/why-we-have-banned-default-exports-and-you-should-do-the-same-d51fdc2cf2ad

  2. In Node.js, modules don't care about the file extension. Truth be told, this is the first time I've ever seen them used at all. My IDE automatically adds imports without extensions too. I wasn't aware browsers even implemented ES6 modules, much less that they complained about missing extensions. I will add them back.

mingos777 commented 5 years ago

BTW, if you decide to phase away from default exports, I can also extend the PR to go through the entire codebase. I only included the FOV module because this is the only module I am using.

ondras commented 5 years ago

BTW, if you decide to phase away from default exports, I can also extend the PR to go through the entire codebase. I only included the FOV module because this is the only module I am using.

Yeah, I will have to think this over. (Also educate myself by reading those articles you linked.)