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

Fix installing XO on npm 6 by bundling TS-dependent dependencies to avoid hoisting #624

Merged
merged 4 commits into from Oct 27, 2021

Conversation

voxpelli
Copy link
Contributor

@voxpelli voxpelli commented Oct 25, 2021

Fixes #555

Ensures that none of the xo dependencies which directly or indirectly depend on typescript gets allowed to be hoisted higher up than the typescript dependency.

More details: #555 (comment)

Should not be shipped until this PR has been merged: xojs/eslint-config-xo-typescript#45

Without the above PR the size of xo will skyrocket to 12.3MB packaged, from current 19.6KB. After above PR it will merely be 1.3MB.


IssueHunt Summary

Referenced issues

This pull request has been submitted to:


Fixes #555

Ensures that none of the xo dependencies that have a direct or indirect dependency on typescript can get hoisted higher up than xo's typescript dependency
index.js Outdated
const eslint = new ESLint(eslintOptions);
const eslint = new ESLint({
...eslintOptions,
resolvePluginsRelativeTo: __dirname
Copy link
Member

@sindresorhus sindresorhus Oct 26, 2021

Choose a reason for hiding this comment

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

Did you see #589 ?

Copy link
Contributor Author

@voxpelli voxpelli Oct 26, 2021

Choose a reason for hiding this comment

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

No, I have to give it a look, generally I think this shouldn't case any issue as it just ensures that plug-in resolution starts from within xo rather than one step up from xo, which helps when hoisting could otherwise have resolving happen that one step up.

As node module resolution traverses upwards all plugins on a higher level should still be found.

@sindresorhus
Copy link
Member

sindresorhus commented Oct 26, 2021

CI is failing. You need to use:

import {fileURLToPath} from 'node:url';
import path from 'node:path';

const __dirname = path.dirname(fileURLToPath(import.meta.url));

Fixing copy and paste error when copying from test setup which didn't use ESM
@voxpelli
Copy link
Contributor Author

voxpelli commented Oct 26, 2021

Fixed @sindresorhus, though apparently you need to approve the workflow again for it to run

@voxpelli
Copy link
Contributor Author

voxpelli commented Oct 27, 2021

Reminder to self: Do not edit things online and always run tests.

Now fixed for real @sindresorhus and everything passes on my local computer. Sorry for the trouble 🙈

@sindresorhus sindresorhus changed the title npm 6 fix: Bundle TS-dependent dependencies to avoid hoisting Fix installing XO on npm 6 by bundling TS-dependent dependencies to avoid hoisting Oct 27, 2021
@sindresorhus sindresorhus merged commit c9bbfb1 into xojs:main Oct 27, 2021
3 checks passed
@sindresorhus
Copy link
Member

sindresorhus commented Oct 27, 2021

Thank you so much for fixing this! 👏 🥳

@voxpelli voxpelli deleted the fix-xo-tsd-conflict-on-npm-6 branch Oct 27, 2021
@sindresorhus
Copy link
Member

sindresorhus commented Oct 27, 2021

@voxpelli
Copy link
Contributor Author

voxpelli commented Oct 27, 2021

On Node 16 😵 So I guess it fixed it for npm 6 but broke it for npm 7

@voxpelli
Copy link
Contributor Author

voxpelli commented Oct 27, 2021

I’ll check when I’m home again in like 15 minutes

@voxpelli
Copy link
Contributor Author

voxpelli commented Oct 27, 2021

@sindresorhus The published tarball for xo@0.46.0 doesn't contain any bundled dependencies, so something has gone wrong when publishing it.

Here's what I get when I run npm pack xo --dry-run:

``` npm notice npm notice 📦 xo@0.46.0 npm notice === Tarball Contents === npm notice 5.7kB cli.js npm notice 65B config/overrides.cjs npm notice 8.3kB config/plugins.cjs npm notice 3.5kB index.js npm notice 3.0kB lib/constants.js npm notice 939B lib/open-report.js npm notice 17.9kB lib/options-manager.js npm notice 1.7kB lib/report.js npm notice 1.1kB license npm notice 2.8kB package.json npm notice 18.9kB readme.md npm notice === Tarball Details === npm notice name: xo npm notice version: 0.46.0 npm notice filename: xo-0.46.0.tgz npm notice package size: 20.1 kB npm notice unpacked size: 64.0 kB npm notice shasum: 6b2113b2e6ec8ac2d814a12108bff882212809b7 npm notice integrity: sha512-O57z234B0L9jK[...]JimWpj0stmfBA== npm notice total files: 11 npm notice ```

And here's what I get when I run npm pack --dry-run on the checked out main of the xo repository:

``` npm notice npm notice 📦 xo@0.46.0 npm notice === Tarball Contents === npm notice 5.7kB cli.js npm notice 65B config/overrides.cjs npm notice 8.3kB config/plugins.cjs npm notice 3.5kB index.js npm notice 3.0kB lib/constants.js npm notice 939B lib/open-report.js npm notice 17.9kB lib/options-manager.js npm notice 1.7kB lib/report.js npm notice 1.1kB license npm notice 2.8kB package.json npm notice 18.9kB readme.md npm notice === Bundled Dependencies === npm notice esrecurse npm notice estraverse npm notice functional-red-black-tree npm notice @nodelib/fs.scandir npm notice @nodelib/fs.stat npm notice @nodelib/fs.walk npm notice @types/json-schema npm notice @typescript-eslint/eslint-plugin npm notice @typescript-eslint/experimental-utils npm notice @typescript-eslint/parser npm notice @typescript-eslint/scope-manager npm notice @typescript-eslint/types npm notice @typescript-eslint/typescript-estree npm notice @typescript-eslint/visitor-keys npm notice array-union npm notice braces npm notice debug npm notice dir-glob npm notice eslint-config-xo-typescript npm notice eslint-scope npm notice eslint-utils npm notice fast-glob npm notice fastq npm notice fill-range npm notice glob-parent npm notice is-extglob npm notice is-glob npm notice is-number npm notice lru-cache npm notice merge2 npm notice micromatch npm notice path-type npm notice picomatch npm notice queue-microtask npm notice regexpp npm notice reusify npm notice run-parallel npm notice semver npm notice to-regex-range npm notice tsutils npm notice yallist npm notice tslib npm notice ms npm notice === Tarball Details === npm notice name: xo npm notice version: 0.46.0 npm notice filename: xo-0.46.0.tgz npm notice package size: 1.2 MB npm notice unpacked size: 6.4 MB npm notice shasum: 569f5b035dfa702136e01a4a5e7878d8d7c45904 npm notice integrity: sha512-hqjo4gP3FA4le[...]eIwTnWNKIHjWg== npm notice bundled deps: 43 npm notice bundled files: 0 npm notice own files: 1774 npm notice total files: 1774 npm notice ```

So I get total files: 1774 when I run it, but for the published version I only get total files: 11

@sindresorhus
Copy link
Member

sindresorhus commented Oct 28, 2021

Turns out to be a bug in npm@7. I upgraded to npm@8 and it showed the correct amount of files now (in xo@0.46.1). However, linting still fails: https://github.com/sindresorhus/ow/runs/4031262058?check_suite_focus=true

@voxpelli
Copy link
Contributor Author

voxpelli commented Oct 28, 2021

Created a fix for it: xojs/eslint-config-xo-typescript#46

@sindresorhus
Copy link
Member

sindresorhus commented Oct 28, 2021

Thanks. That seems to have fixed it. 👏

Sidenote: If you publish without having run npm install, bundledDependencies are silently not included in the package, even with npm@8. Gotta love npm quality engineering...

@voxpelli
Copy link
Contributor Author

voxpelli commented Oct 28, 2021

Don’t you always publish with np @sindresorhus? 😉

@sindresorhus
Copy link
Member

sindresorhus commented Oct 28, 2021

Yes, but could not use it now because of peer dependencies conflicts, so I had to use --force when npm installing.

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