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

Named exports #904

Merged
merged 19 commits into from
Nov 16, 2024
Merged

Named exports #904

merged 19 commits into from
Nov 16, 2024

Conversation

birkskyum
Copy link
Member

@birkskyum birkskyum commented Nov 16, 2024

I left the config files rollup/jest/eslint, but everything else is named exports. I've sometimes just mapped the name, so it's just the import line that changes, without affecting the re-exports, or other logic.

Since this is strictly only an internal change, I don't know if a changelog bullet is needed.

Launch Checklist

  • [X} Confirm your changes do not include backports from Mapbox projects (unless with compliant license) - if you are not sure about this, please ask!
  • Briefly describe the changes in this PR.
  • Link to related issues.
  • Include before/after visuals or gifs if this PR includes visual changes.
  • Write tests for all new functionality.
  • Document any changes to public APIs.
  • Post benchmark scores.
  • Add an entry to CHANGELOG.md under the ## main section.

@codecov-commenter
Copy link

codecov-commenter commented Nov 16, 2024

Codecov Report

Attention: Patch coverage is 98.22785% with 7 lines in your changes missing coverage. Please review.

Project coverage is 92.77%. Comparing base (8cb30d3) to head (1b2dec0).

Files with missing lines Patch % Lines
src/diff.ts 92.59% 2 Missing ⚠️
src/expression/compound_expression.ts 87.50% 2 Missing ⚠️
src/expression/definitions/in.ts 80.00% 1 Missing ⚠️
src/expression/definitions/index_of.ts 80.00% 1 Missing ⚠️
src/expression/index.ts 94.11% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #904      +/-   ##
==========================================
- Coverage   92.79%   92.77%   -0.02%     
==========================================
  Files         105      105              
  Lines        4689     4677      -12     
  Branches     1323     1323              
==========================================
- Hits         4351     4339      -12     
  Misses        338      338              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@birkskyum birkskyum requested a review from HarelM November 16, 2024 17:52
@@ -1,4 +1,4 @@
export default function extendBy(output: any, ...inputs: Array<any>) {
export function extendBy(output: any, ...inputs: Array<any>) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if this can be removed now and we can use some typescript functionality

Copy link
Member Author

@birkskyum birkskyum Nov 16, 2024

Choose a reason for hiding this comment

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

extendBy is doing the same as:

Object.assign(output, ...inputs);

But our eslint settings doesn't like Object.assign
https://eslint.org/docs/latest/rules/no-restricted-properties

Copy link
Collaborator

Choose a reason for hiding this comment

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

Keep it for now then, it's out of scope for this PR...

@HarelM
Copy link
Collaborator

HarelM commented Nov 16, 2024

I think it's a big enough change to add to the changelog, even if it's only refactoring.
I would also look into removing "import * from..." to finish up the cleaning.
Looks great, thanks!!

@birkskyum
Copy link
Member Author

birkskyum commented Nov 16, 2024

The only import * as that are left now are in test files, looping over exports or used in spyOn

Screenshot 2024-11-16 at 20 29 40

@birkskyum birkskyum merged commit 14ecbdf into maplibre:main Nov 16, 2024
6 checks passed
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.

3 participants