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: dependency upgrades and npx bins in builds #1

Closed
wants to merge 1 commit into from

Conversation

blake-regalia
Copy link

The tool is currently broken as it fails to parse:

export declare const TEST: unique symbol;

This PR upgrades dependencies to latest typescript parser versions and uses npx for build taks.

@Lusito
Copy link
Owner

Lusito commented Jul 9, 2021

Thanks for the PR, but I've got to ask first: Why do you want to prefix everything with npx? These commands should work without.

@Lusito
Copy link
Owner

Lusito commented Jul 9, 2021

Also, you've switched to a newer version of npm with the new lockfile format. I've noticed some strange things with the new npm and would like to skip on moving to it until its a bit more stable.

@blake-regalia
Copy link
Author

rimraf, eslint, sort-package-json are all commands to npm packages that package.json just assumes the developer has installed globally. They may not have it installed and linked globally, or they may have a different version of eslint, etc. Using npx is the best practice to ensure the script task will use the bin that was installed locally in node_modules from the devDependencies.

@Lusito
Copy link
Owner

Lusito commented Sep 12, 2021

Sorry about the delay, the past months have been quite busy.

npm will always prefer the commands that are installed in node_modules. npx is a command to optionally download these packages or to use when you run them from command line rather than from a scripts definition. None of the commands used in the scripts need to be globally installed, as they all exist within node_modules.

I've updated idtsc just now, so this PR is obsolete.

@Lusito Lusito closed this Sep 12, 2021
@blake-regalia
Copy link
Author

@Lusito sorry but you are wrong, and I'm just trying to help here. This package does not build for all developers using the provided package.json across all environments, but if you don't care then so be it.

https://stackoverflow.com/a/15157360

@Lusito
Copy link
Owner

Lusito commented Sep 12, 2021

The link you posted it about running a node_modules command from the command line. That is different from running a node_modules command from within a package.json script.

Here is the explanation: https://docs.npmjs.com/cli/v7/using-npm/scripts#path

If you depend on modules that define executable scripts, like test suites, then those executables will be added to the PATH for executing the scripts.

In other words: Since I installed rimraf as a devDependency, it will be added to the PATH and I can run it without using npx when I use the package.json scripts definition. Your stackoverflow link even has an answer which shows this (even though it doesn't explain why this works very well): https://stackoverflow.com/a/28549523

I have done this in a lot of personal and commercial projects and never had any issues with this (across multiple environments: Mac, Linux and Windows).

Now you might argue that I didn't add eslint in the (dev)dependencies, but I added it indirectly via another package, which works exactly the same. Any package, which installs itself as a binary into node_modules/.bin should work this way.

I'm not trying to ignore your argument. I'm just saying, that unless there is some other issue here, it should work perfectly fine as per specification of npm scripts. If it does not work for you, feel free to create an issue with an error message and steps to reproduce.

@Lusito
Copy link
Owner

Lusito commented Sep 12, 2021

One issue I did sometimes have cross-platform is that quotes do not work the same on all platforms. Maybe you are on windows and the single quotes don't work for you?

@Lusito
Copy link
Owner

Lusito commented Sep 12, 2021

I've changed quotes to double-quotes, and added some extra quotes, which should fix possible problems on windows. Feel free to try again on the latest master branch.

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