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

Move sicp.d.ts definitions into the declare module #3

Open
wants to merge 6 commits into
base: hw11-pkd22-23
Choose a base branch
from

Conversation

sashabjorkman
Copy link

To get proper type-checking, we should not augment the sicp import. Instead we should create a new definition. This is done by moving all "declares" into the declare module. If this isn't done then TypeScript will resolve sicp as any. The result is compiling but type-unsafe code which is not useful for the refactoring stage of homework 11.
Please see https://stackoverflow.com/questions/42388217/having-error-module-name-resolves-to-an-untyped-module-at-when-writing-cu as for why this change should be made. Without this change users are shown the following:

Could not find a declaration file for module 'sicp'.  '[...]/mce-typed/node_modules/sicp/dist/sicp.js' implicitly has an 'any' type. Try `npm i --save-dev @types/sicp` if it exists or add a new declaration (.d.ts) file containing `declare module 'sicp';`

This patch makes TypeScript not treat the import as an any value. The patch has not been tested on TS versions older than 4.7.4. It is possible the patch isn't necessary in older releases.
Warmest regards, Alexander Björkman

sashabjorkman and others added 2 commits February 13, 2023 16:48
To get proper type-checking, we must "augment" the `sicp` import as opposed to simply defining an alternate version of it.
@sashabjorkman
Copy link
Author

Improvements have been made to accumulate and list and list_ref. Of special importance is accumulate as it was previously defined to only allow for one argument, while 3 are required to actually use it properly. Giving it a strict definition confuses TypeScript as it doesn't see null as an empty list for some reason. Instead we use the new definition for list() which allows for a zero element list to be created as well. In this way, TS is no longer confused.

@sashabjorkman
Copy link
Author

The definition of error and list was fixed to allow a full range between 0 and many arguments. Previously error and list only allowed up to 1 argument each.

@TobiasWrigstad
Copy link
Contributor

Thanks @sashabjorkman -- I did not see this PR until now. I am hesitant to change the code so close to the HW deadline but will look at this after today. Much appreciated!

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