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

Add support for native TypeScript #5815

Merged
merged 6 commits into from
Jan 14, 2022
Merged

Add support for native TypeScript #5815

merged 6 commits into from
Jan 14, 2022

Conversation

aduth
Copy link
Member

@aduth aduth commented Jan 13, 2022

Why: One of the other hopes with #5746 was a more direct path to incrementally adopting native TypeScript if we choose. The proposed advantage here is improved syntax, Prettier integration, and clarity of inline code documentation in place of the JSDoc-based typings.

**Why**: One of the other advantages / hopes with #5746 was a more direct path to incrementally adopting native TypeScript if we choose.
Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM let's goooo

@aduth aduth changed the title Try: TypeScript Add support for native TypeScript Jan 13, 2022
@aduth aduth marked this pull request as ready for review January 13, 2022 20:05
@aduth
Copy link
Member Author

aduth commented Jan 13, 2022

I need to take a closer look at this logic, which was revised as part of #5746 (not yet deployed to production). Seems like it could already be problematic for .jsx string extraction.

/**
* Given a file name, returns true if the file is a JavaScript file, or false otherwise.
*
* @param {string} filename
*
* @return {boolean}
*/
const isJavaScriptFile = (filename) => filename.endsWith('.js');

Thinking one of either:

  • Add more extensions to that list.
  • See if there's a better way to detect a Webpack chunk as a script.

@aduth
Copy link
Member Author

aduth commented Jan 13, 2022

I need to take a closer look at this logic, which was revised as part of #5746 (not yet deployed to production). Seems like it could already be problematic for .jsx string extraction.

Actually, it turns out this is fine, likely because it's the output having the .js extension which is important. Confirmed via test case in c7af65b, as well as checking for the string in the modified component:

$ rm -rf public/packs && yarn build && ag -o '"idv.troubleshooting.options.doc_capture_tips"' public/packs
...
public/packs/js/document-capture.es.js
1:"idv.troubleshooting.options.doc_capture_tips"

public/packs/js/document-capture.en.js
1:"idv.troubleshooting.options.doc_capture_tips"

public/packs/js/document-capture.fr.js
1:"idv.troubleshooting.options.doc_capture_tips"

@aduth aduth merged commit 399e030 into main Jan 14, 2022
@aduth aduth deleted the aduth-try-typescript branch January 14, 2022 14:45
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