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

chore(dtslint): use npx #4088

Merged
merged 5 commits into from
Sep 4, 2018
Merged

chore(dtslint): use npx #4088

merged 5 commits into from
Sep 4, 2018

Conversation

cartant
Copy link
Collaborator

@cartant cartant commented Sep 3, 2018

Description:

This PR removes the running of dtslint from the full-validation build.

Until #4079 is sorted and dtslint can be installed without pinned dependency versions, it'll be less painful to just switch it off. Having every PR fail is too annoying and having to update the lock file all the time is absurd.

Its running can be re-instated once #4079 fixes the problem.

This PR disables a new dtslint rule that's not needed and removes dtslint from the project's package.json. Instead, it's installed into spec-dtslint-build (which is a git-ignored directory) run using npx, which installs a temporary copy. This should mean that dtslint installs (on Travis, at least) are no longer pinned by the project's lock file - something that has been causing problems.

Related issue (if exists): #4079

@cartant cartant changed the title chore(travis): don't run dtslint chore(dtslint): move install to spec-dtslint-build Sep 3, 2018
@kwonoj
Copy link
Member

kwonoj commented Sep 3, 2018

What if we don't install dtslint in package but try to run via npx? With npx it's possible to install different version of tsc if needed, or let dtslint resolves by their own.

@cartant
Copy link
Collaborator Author

cartant commented Sep 3, 2018

I'm not familiar with npx. If it were to be used, how/where would dtslint be installed? Globally?

Also, regarding a different tsc version, dtslint does something weird and installs/downloads a bunch of different versions of TypeScript (under its own node_modules/dtslint directory) and uses them when it runs. That is, it already runs against a bunch of different TypeScript versions.

I think it's downloading stuff at runtime is somehow related to the problems that are effected by pinning versions in the package.json. But I cannot say that I fully understand what it's doing. It's somewhat infuriating.

@kwonoj
Copy link
Member

kwonoj commented Sep 3, 2018

Npx resolves to its temp (npm cache) location if package is not specified / installed under node_modules. Theoritically it shoudln't affected by local module installation due to its behavior.

@cartant
Copy link
Collaborator Author

cartant commented Sep 3, 2018

Interesting. I'll have a look at it.

@cartant
Copy link
Collaborator Author

cartant commented Sep 3, 2018

Well, it works, but it installs a fresh copy into its cache each time. So it's kinda slow.

@kwonoj
Copy link
Member

kwonoj commented Sep 4, 2018

yeah, but it's ci so I wouldn't be much worried for some slowness. if someone need to run it on local frequently, may need to think better workaround though.

@cartant
Copy link
Collaborator Author

cartant commented Sep 4, 2018

The spec-dtslint-build script fails on Linux due to a quote issue, so I'm going with your npx solution. I can workaround it differently - locally - to avoid the slowness problem.

Thanks.

@cartant cartant changed the title chore(dtslint): move install to spec-dtslint-build chore(dtslint): use npx Sep 4, 2018
Copy link
Member

@kwonoj kwonoj left a comment

Choose a reason for hiding this comment

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

🚢

@pertrai1
Copy link
Contributor

pertrai1 commented Sep 4, 2018

🚢

@benlesh benlesh merged commit 4dde71b into ReactiveX:master Sep 4, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Oct 4, 2018
@cartant cartant deleted the issue-4079 branch September 24, 2020 07:08
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants