Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[REV-40] fix: remove ts type aliases #1295

Merged
merged 3 commits into from
Nov 5, 2020
Merged

Conversation

maksnester
Copy link
Contributor

@maksnester maksnester commented Nov 3, 2020

That's done to avoid having aliases in the production build.

See

Please don't hate me for that 😅

put it on hold for 1.2.1


Just to summarise, currently, we use @ alias in imports. And they are left unchanged in resulting type declarations. And that is not good because some types are just replaced with any and it doesn’t work at all in angular-cli based setups (failed with compile-time errors that fairly blame aliases).

There are 3 solutions for that

  1. Just use relative paths (PR ☝️ ) - no cons for the resulting package, only some headache for you as developers because you will see ../../../../../ a lot
  2. Instead of using @ alias we can use @cognite/reveal alias. It works fine, but it breaks the perfectly valid (though more rare) case when the package is installed under a different name, e.g. package.json has this "my-reveal-v1.2": "npm:@cognite/reveal@1.2.0" I personally use such things from time to time 🤷‍♂️ so for this case the problem remains unfixed.
  3. Use some transformation plugin for typescript in pair with an alternative compiler (e.g. ttypescript). It requires replacement of the compiler and probably locking of typescript version on 3.5. I didn’t make it work when tried, because I used it with our current version (4.0.5) and that didn’t work. It should work fine, but IMO that option is bad because of ts version locking and it just feels unreliable when some third-party package uses something from TSC that not really supposed to be used.

@maksnester maksnester requested a review from a team as a code owner November 3, 2020 15:03
@github-actions
Copy link

github-actions bot commented Nov 3, 2020

There were failures in the examples workflow. This usually means a visual regression test has failed. Image diffs for visual tests can be downloaded as an artifact here. If there are no artifacts there's an error somewhere else in the examples workflow. If you have made intentional changes you can update the image snapshots by running yarn snapshots:update in examples/

@github-actions
Copy link

github-actions bot commented Nov 3, 2020

📙 Documentation preview is available from
https://cognitedata.github.io/reveal-docs-preview/1295/docs/next/.

That's done to avoid having aliases in production build.

See
* https://stackoverflow.com/a/57290924/2670121
* microsoft/TypeScript#10866
@maksnester maksnester force-pushed the mn/ts-aliases-removed branch from 3fd51e7 to 4aae304 Compare November 3, 2020 15:11
@codecov
Copy link

codecov bot commented Nov 3, 2020

Codecov Report

Merging #1295 into master will not change coverage.
The diff coverage is 98.41%.

@@           Coverage Diff           @@
##           master    #1295   +/-   ##
=======================================
  Coverage   64.00%   64.00%           
=======================================
  Files          97       97           
  Lines        4414     4414           
  Branches      425      425           
=======================================
  Hits         2825     2825           
  Misses       1585     1585           
  Partials        4        4           
Impacted Files Coverage Δ
viewer/src/datamodels/cad/createCadManager.ts 85.71% <ø> (ø)
.../src/datamodels/cad/rendering/postRenderEffects.ts 0.00% <ø> (ø)
viewer/src/datamodels/cad/rendering/shaders.ts 100.00% <ø> (ø)
viewer/src/datamodels/cad/sector/types.ts 100.00% <ø> (ø)
...wer/src/datamodels/pointcloud/PointCloudFactory.ts 19.23% <ø> (ø)
...wer/src/datamodels/pointcloud/PointCloudManager.ts 56.00% <ø> (ø)
...wer/src/datamodels/pointcloud/PotreeLoadHandler.ts 100.00% <ø> (ø)
...c/datamodels/pointcloud/createPointCloudManager.ts 81.81% <ø> (ø)
viewer/src/index.ts 0.00% <ø> (ø)
viewer/src/migration.ts 100.00% <ø> (ø)
... and 34 more

@github-actions
Copy link

github-actions bot commented Nov 3, 2020

There were failures in the examples workflow. This usually means a visual regression test has failed. Image diffs for visual tests can be downloaded as an artifact here. If there are no artifacts there's an error somewhere else in the examples workflow. If you have made intentional changes you can update the image snapshots by running yarn snapshots:update in examples/

@maksnester maksnester force-pushed the mn/ts-aliases-removed branch from 233b48e to c760bf5 Compare November 3, 2020 15:40
@maksnester maksnester marked this pull request as draft November 4, 2020 10:09
@maksnester maksnester marked this pull request as ready for review November 5, 2020 08:48
@github-actions
Copy link

github-actions bot commented Nov 5, 2020

There were failures in the examples workflow. This usually means a visual regression test has failed. Image diffs for visual tests can be downloaded as an artifact here. If there are no artifacts there's an error somewhere else in the examples workflow. If you have made intentional changes you can update the image snapshots by running yarn snapshots:update in examples/

@cognite-bulldozer cognite-bulldozer bot merged commit be72a44 into master Nov 5, 2020
@cognite-bulldozer cognite-bulldozer bot deleted the mn/ts-aliases-removed branch November 5, 2020 09:49
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants