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

Pre-commit hooks are failing in rosetta. #445

Closed
pixelzoom opened this issue Dec 17, 2024 · 5 comments
Closed

Pre-commit hooks are failing in rosetta. #445

pixelzoom opened this issue Dec 17, 2024 · 5 comments
Assignees

Comments

@pixelzoom
Copy link
Contributor

pixelzoom commented Dec 17, 2024

After a pull-all, pre-commit hooks were failing for me with:

../../../../rosetta/src/server/app.ts:16:21 - error TS7016: Could not find a declaration file for module ‘express’. ‘/Users/cmalley/PhET/GitHub/rosetta/node_modules/express/index.js’ implicitly has an ‘any’ type.
Try npm i --save-dev @types/express if it exists or add a new declaration (.d.ts) file containing declare module ‘express’;
16 import express from ‘express’;
~~~~~~~~~
../../../../rosetta/src/server/app.ts:21:26 - error TS2307: Cannot find module ‘../common/publicConfig.js’ or its corresponding type declarations.
21 import publicConfig from ‘../common/publicConfig.js’;
~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../../../rosetta/src/server/app.ts:44:14 - error TS7006: Parameter ‘req’ implicitly has an ‘any’ type.
44 app.use( ( req, res, next ) => {

In Slack#dev-public, @samreid said::

Did you npm install there?
My rosetta problem is:
~/phet/root/rosetta$ grunt type-check
Running "type-check" task
(Forwarding task to perennial-alias)
Running "type-check" task
src/server/app.ts:21:26 - error TS2307: Cannot find module '../common/publicConfig.js' or its corresponding type declarations.

21 import publicConfig from '../common/publicConfig.js';
~~~~~~~~~~~~~~~~~~~~~~~~~~~
Found 1 error.

Done.

@zepumph and @jbphet are apparently working on making rosetta TypeScript capable, and this is related.

I pointed out that having to npm install in a repo that is not related to build tools is not something that is expected. And it was not announced.

After doing cd rosetta; npm install as suggested, I'm still getting the failure that @samreid noted above. I find no file named publicConfig.js. Is it something new that was not checked in?

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Dec 17, 2024

Besides the publicConfig.js error...

Requiring npm install in a repo other than chipper/perennial/perennial-alias is a slippery slope. Very inconvenient, complicates developer instructions, complicates SceneryStack, ...

@jbphet
Copy link
Contributor

jbphet commented Dec 17, 2024

The problem is rooted in the fact that the publicConfig.js file is actually a build artifact. We'll have to figure out how to make this work. And I agree that it's not great to have to do an npm install in rosetta for this to work.

After consulting with @zepumph and @samreid, a commit was made that basically turns off TypeScript type checking for rosetta in order to fix the immediate problem. We'll work on the larger issue of making type checking work for Rosetta later, and under #311.

@zepumph - Please double check that pre-commit hooks are now working for you too and, if so, I think we can close this.

@jbphet jbphet removed their assignment Dec 17, 2024
@zepumph
Copy link
Member

zepumph commented Dec 17, 2024

Looks great. Thanks

@zepumph zepumph closed this as completed Dec 17, 2024
@zepumph
Copy link
Member

zepumph commented Dec 17, 2024

Hmm, this probably isn't a real solution, since lint is now failing since app.ts is not able to get type info. @jbphet should we just proceed with getting all js files committed, and generate a json as a build artifact?

@zepumph zepumph reopened this Dec 17, 2024
@zepumph zepumph assigned jbphet and unassigned zepumph Dec 17, 2024
@zepumph zepumph transferred this issue from phetsims/chipper Dec 17, 2024
jbphet added a commit that referenced this issue Dec 19, 2024
@jbphet
Copy link
Contributor

jbphet commented Dec 19, 2024

I have reverted app.ts back to app.js, so it is now passing lint. I just checked CT, and rosetta is now green. I think it's now safe to close this, and we will pick things up again under #311 when we have time to do so.

@jbphet jbphet closed this as completed Dec 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants