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

repo: update typescript references #7096

Merged
merged 3 commits into from
Feb 11, 2020
Merged

repo: update typescript references #7096

merged 3 commits into from
Feb 11, 2020

Conversation

paul-marechal
Copy link
Member

@paul-marechal paul-marechal commented Feb 6, 2020

Signed-off-by: Paul Maréchal paul.marechal@ericsson.com

What it does

  1. Update typescript references.
  2. Add a CI script to check if typescript references are in sync.

How to test

  • Run yarn test:references: it should work.
  • Change a reference in any package to something bogus (non-existant path).
  • Run yarn test:references: it should fail.

@paul-marechal
Copy link
Member Author

Not sure if we should automate the script to compile references?

@vince-fugnitto
Copy link
Member

Not sure if we should automate the script to compile references?

I think so, how will we know if a reference is missing? (how did we know in this case)

@paul-marechal
Copy link
Member Author

paul-marechal commented Feb 6, 2020

@vince-fugnitto indeed, added a script to check if everything is in sync while running the tests. People will need to manually trigger the update script to make sure everything is in order if the check fails. On travis it will fail since there will be uncommitted changes then.

@paul-marechal paul-marechal force-pushed the mp/ref-update branch 2 times, most recently from 460155e to 48e0f3b Compare February 6, 2020 22:08
@akosyakov
Copy link
Member

akosyakov commented Feb 7, 2020

Why don't run references generation during prepare always, similarly how we prepare travis file when new project is added? Without dry runs and so on, one script which always rewrites references configuration to the proper state.

@akosyakov akosyakov added the dependencies pull requests that update a dependency file label Feb 7, 2020
@paul-marechal
Copy link
Member Author

paul-marechal commented Feb 7, 2020

@akosyakov the reason is that I believe human supervision is required should the script run. The script guaranties that our workspace layout it followed by TypeScript, but we can also add specific references that are out of this system and I'm "afraid" for those. Also I don't like scripts that crawl the disk to run when not needed, especially when they can modify your workspace.

edit: Looking into it, I guess I could make it like you said. I assumed people might want control over the references, but I imagine not.

@paul-marechal
Copy link
Member Author

Updated, fingers crossed that the script will behave. Worst case ping me if something goes wrong.

@paul-marechal paul-marechal force-pushed the mp/ref-update branch 3 times, most recently from 28bff70 to 6ff8096 Compare February 7, 2020 16:05
@paul-marechal paul-marechal changed the title plugin-ext: update typescript references repo: update typescript references Feb 7, 2020
@paul-marechal paul-marechal force-pushed the mp/ref-update branch 2 times, most recently from 04df44d to e40796c Compare February 7, 2020 21:08
@akosyakov
Copy link
Member

akosyakov commented Feb 10, 2020

the reason is that I believe human supervision is required should the script run

@marechal-p we can supervise output of a script and fix the script to behave. I don't want anything complex and additional npm scripts, script options and so on. It should establish links for all workspaces each time, we have another script to fail the build if somebody forgot to supervise output.

@paul-marechal
Copy link
Member Author

@akosyakov that's done. The script now sets all ts references between our local repos, and only that. People shouldn't edit the references manually as it might get overridden by the script. Should someone think we need a different set of references, the script would have to be updated. Is this ok?

@akosyakov
Copy link
Member

Should someone think we need a different set of references, the script would have to be updated. Is this ok?

yes, i think it is fine, we should discuss if someone wants something like that anyway, thank you

@@ -1,3 +1,6 @@
// @ts-check
Copy link
Member

Choose a reason for hiding this comment

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

we should have copyright here?

Copy link
Member

@akosyakov akosyakov Feb 11, 2020

Choose a reason for hiding this comment

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

ah, see it is below, not sure that it is correct, but anyway

Copy link
Member Author

Choose a reason for hiding this comment

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

// @ts-check has to be at the top for it to work apparently

@akosyakov
Copy link
Member

looks good to me, @marcdumais-work @vince-fugnitto could you have a look as well?

Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

I tested that the two scripts work correctly when there are outdated references:

yarn test:references

Result
(mp/ref-update ✓) theia yarn test:references
yarn run v1.19.1
$ node scripts/compile-references --dry-run
TypeScript references seem to be out of sync, run "yarn prepare:references" to fix.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

yarn prepare:references

Result
(mp/ref-update U:1 ✗) theia yarn prepare:references
yarn run v1.19.1
$ node scripts/compile-references.js
info: /home/evinfug/workspace/theia/examples/electron/compile.tsconfig.json updated.
TypeScript references were out of sync and got updated.
Done in 0.49s.

I also verified that the current state contains no outdated references 👍

@paul-marechal
Copy link
Member Author

paul-marechal commented Feb 11, 2020

@vince-fugnitto I just improved logging a bit, thanks to your output.

I will now rebase, run the scripts again to see if anything needs updating, and finally merge when CI turns green.

Build automatically checks and updates references before running tsc.

Add 2 new npm scripts:

- `prepare:references`: updates the references, if required.
- `test:references`: fails if references are out of sync.

Signed-off-by: Paul Maréchal <paul.marechal@ericsson.com>
Signed-off-by: Paul Maréchal <paul.marechal@ericsson.com>
Everytime I open the `.travis.yml` file VS Code fixes indentation caused
by our prepare script.

Signed-off-by: Paul Maréchal <paul.marechal@ericsson.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants