Skip to content

Conversation

@florian-sanders-cc
Copy link
Contributor

What does this PR do?

  • Adds an ADR about Type Checking.

@github-actions
Copy link
Contributor

🔎 A preview has been automatically published : https://clever-components-preview.cellar-c2.services.clever-cloud.com/docs/adr-typechecking/index.html.

This preview will be deleted once this PR is closed.

Copy link
Member

@roberttran-cc roberttran-cc left a comment

Choose a reason for hiding this comment

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

Great ADR!

A couple small corrections to make. 😉

Copy link
Contributor

@HeleneAmouzou HeleneAmouzou left a comment

Choose a reason for hiding this comment

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

LGTM ! 👏 Nothing more to add than the comments @roberttran-cc already made !

Copy link
Member

@hsablonniere hsablonniere left a comment

Choose a reason for hiding this comment

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

Great ADR!!! 👏

  • The build step is only here to optimize the code of the components.

Not sure if we really want to but there's an opportunity to mention bare imports. Our source files need a bundling phase to handle such bare imports. Nowadays, they could work as is but with import maps.

Include a note about this if you think it's relevant 😉

Copy link
Member

@Galimede Galimede left a comment

Choose a reason for hiding this comment

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

The ADR is great, GG Florian! 💪

I have maybe a small suggestion, we do refer to some tools like lit-analyzer or CEM, maybe we could add a section with a link to these projects or add a link directly where they are first mentioned? 🤔

Copy link
Contributor

@pdesoyres-cc pdesoyres-cc left a comment

Choose a reason for hiding this comment

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

LGTM. Well done Florian.

@florian-sanders-cc
Copy link
Contributor Author

@hsablonniere

Not sure if we really want to but there's an opportunity to mention bare imports. Our source files need a bundling phase to handle such bare imports. Nowadays, they could work as is but with import maps.

Include a note about this if you think it's relevant 😉

Good idea! I always forget about import maps because last time I played with them I found them not very convenient.
I've added a sentence to mention bare imports & the fact we don't use import maps because we already need the bundler to optimize the code anyway.

@florian-sanders-cc
Copy link
Contributor Author

@Galimede

I have maybe a small suggestion, we do refer to some tools like lit-analyzer or CEM, maybe we could add a section with a link to these projects or add a link directly where they are first mentioned? 🤔

Good idea! I chose to add direct links to the projects within the sentences instead of adding a separate section

@florian-sanders-cc florian-sanders-cc dismissed roberttran-cc’s stale review January 10, 2025 11:41

fixes have been applied (thanks again!), no need to check again

@florian-sanders-cc florian-sanders-cc merged commit 7de9cd2 into master Jan 10, 2025
4 checks passed
@florian-sanders-cc florian-sanders-cc deleted the docs/adr-typechecking branch January 10, 2025 11:42
@github-actions
Copy link
Contributor

🔎 The preview has been automatically deleted.

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.

6 participants