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

Refactor namespaces to modules #50

Merged
merged 2 commits into from
Jan 12, 2023
Merged

Refactor namespaces to modules #50

merged 2 commits into from
Jan 12, 2023

Conversation

ejizba
Copy link
Contributor

@ejizba ejizba commented Jan 6, 2023

Tangentially related to #12, a lot of places (example here and here) say not to use namespaces for organizational reasons. Instead, they say use modules and re-export where necessary. In other words, instead of something like this:

export namespace app { ... }

Just put the code in a separate file and re-export it like this:

export * as app from './app';

At least, that's my interpretation. Functionally, this makes very little difference to our package (no changes necessary to the prototype). The PR is mostly just splitting code into separate files.

@ejizba
Copy link
Contributor Author

ejizba commented Jan 12, 2023

Fwiw, I did split this up into separate commits to make it easier to review. The first commit just moves the code. The second commit is what actually converted to namespaces and is the interesting piece to review. It should also be viewed with whitespace changes off: 920bbab?w=1

Copy link
Contributor

@hossam-nasr hossam-nasr left a comment

Choose a reason for hiding this comment

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

LGTM! I also like that this reduces the length of our index.ts and index.d.ts and nicely separates code :)

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