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

Deprecate checkAndInstallPackage and fetchPackage #1247

Closed
ringods opened this issue Aug 26, 2020 · 2 comments · Fixed by #1263 or #1266
Closed

Deprecate checkAndInstallPackage and fetchPackage #1247

ringods opened this issue Aug 26, 2020 · 2 comments · Fixed by #1263 or #1266
Labels

Comments

@ringods
Copy link
Contributor

ringods commented Aug 26, 2020

I'm hearing from the core maintainers that everything in Patternlab should be(come) more explicit. In the uikit handling (#1225, #1246), I'm making the package name and explicit requirement and I'm loading resources via de NodeJS resolver.

NPM, Yarn v1, Yarn v2 and pnpm don't all have the same "package" lookup strategy. Yarn v2 with PnP even wants to get rid of the complete node_modules folder(s). The reason to work via the NodeJS resolver is that it doesn't matter anymore which package manager our users are using.

As part of the above work, I also bumped into an issue with starterkits. The CLI has the option to install a starterkit based on the package name. But if it isn't found, there is a check to detect which package manager is used and a subprocess will install the required package. But installing the package in a subprocess will, given the different package managers, not always make the package available in the parent process.

So, a starterkit should be a dependency of the project at hand if the lookup happens via the resolver. That is why I want to get rid of the functions checkAndInstallPackage and fetchPackage:

/**
* @func fetchPackage
* @desc Fetches and saves packages from npm into node_modules and adds a reference in the package.json under dependencies
* @param {string} packageName - The package name
*/
const fetchPackage = packageName =>
wrapAsync(function*() {
const useYarn = hasYarn();
const pm = useYarn ? 'yarn' : 'npm';
const installCmd = useYarn ? 'add' : 'install';
try {
if (packageName) {
const cmd = yield spawn(pm, [installCmd, packageName]);
error(cmd.stderr);
}
} catch (err) {
error(
`fetchPackage: Fetching required dependencies from ${pm} failed for ${packageName} with ${err}`
);
throw err; // Rethrow error
}
});
/**
* @func checkAndInstallPackage
* Checks whether a package for a given packageName is installed locally. If package cannot be found, fetch and install it
* @param {string} packageName - The package name
* @return {boolean}
*/
const checkAndInstallPackage = packageName =>
wrapAsync(function*() {
try {
require.resolve(packageName);
return true;
} catch (err) {
debug(
`checkAndInstallPackage: ${packageName} not installed. Fetching it now …`
);
yield fetchPackage(packageName);
return false;
}
});

@sghoweri @JosefBredereck

@ringods
Copy link
Contributor Author

ringods commented Sep 7, 2020

@JosefBredereck @mfranzke @sghoweri due to the resolver changes I'm slowly integrating, bugs are reported (#1257) which expose exactly the scenario as described in this issue: npm create pattern-lab installs packages in the project folder from a child node process and immediately afterwards tries to resolve these from the parent node process.

This fails because the resolver loads from a temporary folder like $HOME/.npm/_npx/35338/lib/node_modules/. Every run is a different temp folder which doesn't have the @pattern-lab/edition-node package installed.

When I first run npm init, followed by npm install create-pattern-lab, it runs correctly because both run from the same node_modules folder.

However, this has an impact on the global user experience. A workaround for now could be that the create-pattern-lab package would only modify the package.json file of the project being created with the dependencies needed based on the user input, but that no resources are copied yet. After the npm create pattern-lab "wizard", the user should execute npm install. We add the correct post-install hook script to some of our packages that copy the resources at that point in time.

Feedback appreciated.

@JosefBredereck
Copy link
Contributor

It could modify the package.json and afterward with start an npm install in that directory. But I don't know if we could easily archive that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants