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

reorganised imports for FOV module #153

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mingos777
Copy link
Contributor

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
Copy link
Owner

ondras commented Dec 19, 2018

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
Copy link
Owner

ondras commented Dec 19, 2018

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 #148 (comment). So this is already explained.

@mingos777
Copy link
Contributor Author

  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
Copy link
Contributor Author

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
Copy link
Owner

ondras commented Dec 19, 2018

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.)

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.

2 participants