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 MTS and CTS Support #244

Merged
merged 6 commits into from
Jun 5, 2024
Merged

Add MTS and CTS Support #244

merged 6 commits into from
Jun 5, 2024

Conversation

elliot-huffman
Copy link
Collaborator

@elliot-huffman elliot-huffman commented May 30, 2024

This PR adds MTS and CTS support for file output and input so that projects that use mixed module modes are supported.

Currently when an MTS or CTS file is used, the following error is encountered:
Internal Error: sourcePath and outFilePath are identical: ...

Add support for MTS and CTS files if detected, otherwise fall back to standard TS output.
Add comments and JS Docs to refactored function.
Add return type to make function typing explicit.
Switch to Unicode regex to support multi-language filesystems and emoji (when FSs eventually all get UTF8 support).
Add support to the --import-guards functionality to make sure that it is feature paired with the new code for the file outputting logic.
@elliot-huffman elliot-huffman changed the title Add MTS and CTS Supprot Add MTS and CTS Support May 30, 2024
@rhys-vdw
Copy link
Owner

rhys-vdw commented Jun 3, 2024

Hi @elliot-huffman, this looks good to me.

Could you deduplicate the regex used for the file extensions? Declare it as a constant regex. Slight oversight, you've made one case insensitive but not the other. DRYing this would help such mistakes in future.

Also you'll need to prettier format for the tests to pass. You can npm run format (or use IDE extension of your choice).

Once this is merged I'll add you on as a collaborator if you would like it, so let me know.

Thank you for your contribution!

Migrate to string primitive methods to reduce risk of breach and improve performance.
@elliot-huffman
Copy link
Collaborator Author

elliot-huffman commented Jun 4, 2024

Hi @elliot-huffman, this looks good to me.

Could you deduplicate the regex used for the file extensions? Declare it as a constant regex. Slight oversight, you've made one case insensitive but not the other. DRYing this would help such mistakes in future.

Also you'll need to prettier format for the tests to pass. You can npm run format (or use IDE extension of your choice).

Once this is merged I'll add you on as a collaborator if you would like it, so let me know.

Thank you for your contribution!

I would love that!
QQ, when you say de-duplicate, I only have one Regex added to the script.
What is it duplicating?

As a precaution, I have replaced the regex with more preformat and less risk of regex compromise, string primitive methods.
This should reduce memory requirements, cpu cycles, as well as reduce the risk of a new type of regular expression attack.

@rhys-vdw
Copy link
Owner

rhys-vdw commented Jun 5, 2024

QQ, when you say de-duplicate, I only have one Regex added to the script. What is it duplicating?

@elliot-huffman Sorry, I should have been more specific! I've added line comments. You didn't add the regexes, but you've caused them to deviate.

I would prefer to see:

// somewhere near top of file
const fileExtensionRegex = /\.(ts|mts|cts|tsx|d\.ts)$/u

To ensure that they keep in sync in future.

I would love that!

Inviting you now.

Remove the duplicate code that the file matcher logic uses and have it at the top of the entry file instead of declared inline.
Make sure that all regular expressions are Unicode compatible and that they are case insensitive matches.
This behavior that should be caught on the Windows platform, as Windows is case insensitive and this could cause a namespace collision if not insensitive matching.
While possible to have two different files with the same name only differing on linux and MacOS, this is generally agreed upon as a bad practice.
@elliot-huffman elliot-huffman requested a review from rhys-vdw June 5, 2024 02:20
@rhys-vdw rhys-vdw merged commit 1ea94de into rhys-vdw:master Jun 5, 2024
2 checks passed
@rhys-vdw
Copy link
Owner

rhys-vdw commented Jun 5, 2024

Nice one.

@elliot-huffman elliot-huffman deleted the patch-1 branch June 5, 2024 06:15
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.

2 participants