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

fix: type issue pulling from src/index #645

Merged
merged 4 commits into from
Apr 22, 2020

Conversation

nickofthyme
Copy link
Collaborator

@nickofthyme nickofthyme commented Apr 22, 2020

Summary

The typecheck for the built library includes surrounding files and directories, which when running the check ../../src may resolve to a module but this file is outside of the dist directory and will fail when checked in kibana.

To prevent this in the future I added a linting rule to prevent imports from src/index.ts and any parent directory/files of src.

Also, fix tsconfig check to error on references outside of the dist folder.

- add import/no-restricted-paths rule
- prevent import anywhere in src/ from src/index.ts
- prevent import anywhere in src outside of src
@nickofthyme nickofthyme added bug Something isn't working :build Build tools / dependencies labels Apr 22, 2020
@nickofthyme nickofthyme requested review from markov00 and rshen91 April 22, 2020 19:25
@codecov-io
Copy link

codecov-io commented Apr 22, 2020

Codecov Report

Merging #645 into master will increase coverage by 0.49%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #645      +/-   ##
==========================================
+ Coverage   72.02%   72.52%   +0.49%     
==========================================
  Files         246      259      +13     
  Lines        8165     8411     +246     
  Branches     1593     1633      +40     
==========================================
+ Hits         5881     6100     +219     
- Misses       2253     2276      +23     
- Partials       31       35       +4     
Impacted Files Coverage Δ
src/chart_types/xy_chart/utils/specs.ts 100.00% <ø> (ø)
src/mocks/series/series_identifiers.ts 100.00% <ø> (ø)
src/mocks/utils.ts 94.11% <100.00%> (ø)
src/index.ts
src/mocks/scale/index.ts 100.00% <0.00%> (ø)
src/mocks/scale/scale.ts 77.77% <0.00%> (ø)
src/mocks/hierarchical/palettes.ts 100.00% <0.00%> (ø)
src/mocks/geometries.ts 86.84% <0.00%> (ø)
src/mocks/index.ts 100.00% <0.00%> (ø)
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b3212b0...646a88a. Read the comment docs.

Copy link
Contributor

@rshen91 rshen91 left a comment

Choose a reason for hiding this comment

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

LGTM - doesn't affect stories being rendered (tested locally on Chrome). Es lint rule set up correctly and errors as expected when adding import '../../../scripts/custom_matchers'; in the file elastic-charts/src/utils/themes/colors.ts

Copy link
Member

@markov00 markov00 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've had the same issue today within my test caused by importing something directly from the src/index.ts

@nickofthyme nickofthyme merged commit 3f3a996 into elastic:master Apr 22, 2020
@nickofthyme nickofthyme deleted the fix/dist-types branch April 22, 2020 21:02
markov00 pushed a commit that referenced this pull request Apr 22, 2020
## [18.4.1](v18.4.0...v18.4.1) (2020-04-22)

### Bug Fixes

* type issue pulling from src/index ([#645](#645)) ([3f3a996](3f3a996))
@markov00
Copy link
Member

🎉 This PR is included in version 18.4.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@markov00 markov00 added the released Issue released publicly label Apr 22, 2020
AMoo-Miki pushed a commit to AMoo-Miki/OpenSearch-Dashboards that referenced this pull request Feb 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working :build Build tools / dependencies released Issue released publicly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants