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

Get files to be packed via npm pack --dry-run --json #682

Merged
merged 18 commits into from
Apr 5, 2023

Conversation

tommy-mitchell
Copy link
Collaborator

@tommy-mitchell tommy-mitchell commented Mar 31, 2023

Per #669 (comment).

This PR removes the custom logic for determining a package's files and uses the built-in npm pack --dry-run --json command instead.

While it should just "work", I've added fixtures for a few different cases just to be sure.

Fixes #669.
Fixes #629.
Fixes #615.
Possibly fixes #610.
Possibly fixes #588.

@tommy-mitchell
Copy link
Collaborator Author

This integration test needs to be updated with a fixture:

test.serial('file `new` to package without tags added', async t => {
	await execa('touch', ['new']);
	await execa('touch', ['index.js']);

	t.context.teardown = () => {
		fs.unlinkSync('new');
		fs.unlinkSync('index.js');
	};

	t.deepEqual(await util.getNewFiles({files: ['index.js']}), {unpublished: ['new'], firstTime: ['index.js']});
});

@tommy-mitchell
Copy link
Collaborator Author

tommy-mitchell commented Mar 31, 2023

I've updated the integration test, but I'm not sure how to update it for the CI.

// using `del` v6.0.0

test.serial('file `new` to package without tags added', async t => {
	const pkg = {
		name: 'foo',
		version: '0.0.0',
		files: ['*.js']
	};

	await execa('mkdir', ['fixture']);
	process.chdir('fixture');

	await execa('touch', ['new']);
	await execa('touch', ['index.js']);
	await execa('echo', [`'${JSON.stringify(pkg, undefined, 4)}'`, '>', 'package.json'], {shell: true});

	t.context.teardown = async () => {
		process.chdir('..');
		await del('fixture');
	};

	t.deepEqual(await util.getNewFiles(), {unpublished: ['new'], firstTime: ['index.js', 'package.json']});
});

@sindresorhus
Copy link
Owner

I've updated the integration test, but I'm not sure how to update it for the CI.

I'm not sure either. I didn't add it. I suggest just marking it as .failing for now.

@sindresorhus
Copy link
Owner

CI is still failing

test/new-files.js Outdated Show resolved Hide resolved
@tommy-mitchell
Copy link
Collaborator Author

I've rebased to main for ESM, still working on updating the integration tests.

test/integration.js Outdated Show resolved Hide resolved
@tommy-mitchell
Copy link
Collaborator Author

Still having trouble mocking process.cwd() for the integration tests to be the temp dir, but the esmock issue was fixed so that's not the problem anymore.

@tommy-mitchell
Copy link
Collaborator Author

If I manually set the execa commands in git-util.s and npm/util.js with {cwd: process.cwd()}, then the tests pass. This is a fun one.

@tommy-mitchell
Copy link
Collaborator Author

Ended up modifying execa and pkg-dir for the tests to set the working directory to the temp dir.

@sindresorhus sindresorhus merged commit a6ce792 into sindresorhus:main Apr 5, 2023
@sindresorhus
Copy link
Owner

🎉

@tommy-mitchell Are you planning any more changes or should I do the major release?

@tommy-mitchell
Copy link
Collaborator Author

tommy-mitchell commented Apr 5, 2023

@sindresorhus going to try tackling a couple of small issues, just opened another PR

Edit:

It seems like the new files check causes the initial startup of np to be slower than the latest release. It might have something be this double filter, but I've not profiled or anything:

// source/util.js
export const getNewFiles = async () => {
	const listNewFiles = await gitUtil.newFilesSinceLastRelease();
	const listPkgFiles = await npmUtil.getFilesToBePacked();

	return {
		unpublished: listNewFiles.filter(file => !listPkgFiles.includes(file) && !file.startsWith('.git')),
		firstTime: listNewFiles.filter(file => listPkgFiles.includes(file)),
	};
};

npmUtil.getFilesToBePacked should probably also used the already parsed package.json path:

// source/npm/util.js
export const getFilesToBePacked = async () => {
	// TODO: use pkgPath
	const {stdout} = await execa('npm', ['pack', '--dry-run', '--json'], {cwd: await packageDirectory()});

	const {files} = JSON.parse(stdout).at(0);
	return files.map(file => file.path);
};

Either way, if any pre-UI checks are added, the "time-to-interactive" will just get slower. Maybe we can add an ora spinner to at least show np is working?

Vylpes pushed a commit to Vylpes/random-bunny that referenced this pull request Sep 11, 2023
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [np](https://github.com/sindresorhus/np) | devDependencies | major | [`^7.7.0` -> `^8.0.0`](https://renovatebot.com/diffs/npm/np/7.7.0/8.0.4) |

---

### Release Notes

<details>
<summary>sindresorhus/np (np)</summary>

### [`v8.0.4`](https://github.com/sindresorhus/np/releases/tag/v8.0.4)

[Compare Source](sindresorhus/np@v8.0.3...v8.0.4)

-   Handle first time display of dependencies ([#&#8203;707](sindresorhus/np#707))  [`3f43d78`](sindresorhus/np@3f43d78)

### [`v8.0.3`](https://github.com/sindresorhus/np/releases/tag/v8.0.3)

[Compare Source](sindresorhus/np@v8.0.2...v8.0.3)

-   Fix skipping publish step ([#&#8203;706](sindresorhus/np#706))  [`51dcc2d`](sindresorhus/np@51dcc2d)

### [`v8.0.2`](https://github.com/sindresorhus/np/releases/tag/v8.0.2)

[Compare Source](sindresorhus/np@v8.0.1...v8.0.2)

-   Fix publish not working with Yarn  [`3d448c2`](sindresorhus/np@3d448c2)
-   Include stack trace in errors  [`12fce88`](sindresorhus/np@12fce88)

### [`v8.0.1`](https://github.com/sindresorhus/np/releases/tag/v8.0.1)

[Compare Source](sindresorhus/np@v8.0.0...v8.0.1)

-   Fix a crash in the new dependency check  [`beb7db1`](sindresorhus/np@beb7db1)

### [`v8.0.0`](https://github.com/sindresorhus/np/releases/tag/v8.0.0)

[Compare Source](sindresorhus/np@v7.7.0...v8.0.0)

##### Breaking

-   Require Node.js 16 ([#&#8203;683](sindresorhus/np#683))  [`72879e0`](sindresorhus/np@72879e0)

##### Improvements

-   Add 2FA support for npm version 9+ ([#&#8203;693](sindresorhus/np#693))  [`9cb4bfd`](sindresorhus/np@9cb4bfd)
-   Improve startup time ([#&#8203;688](sindresorhus/np#688))  [`eba203f`](sindresorhus/np@eba203f)
-   Improve the reliability of detecting which files will be included in the package ([#&#8203;682](sindresorhus/np#682))  [`a6ce792`](sindresorhus/np@a6ce792)
-   Add check for new dependencies ([#&#8203;681](sindresorhus/np#681))  [`6867fb9`](sindresorhus/np@6867fb9)

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC43NC4yIiwidXBkYXRlZEluVmVyIjoiMzYuODkuMCIsInRhcmdldEJyYW5jaCI6ImRldmVsb3AifQ==-->

Reviewed-on: https://gitea.vylpes.xyz/RabbitLabs/random-bunny/pulls/67
Co-authored-by: Renovate Bot <[email protected]>
Co-committed-by: Renovate Bot <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment