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.

Sign up for GitHub

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

Jump to bottom

chore: remove external from config #4355

Merged
merged 15 commits into from Jan 21, 2022
Merged

chore: remove external from config #4355

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
73eddfd
chore: remove external from config
dnalborczyk Jan 19, 2022
6fe43ae
simplify named imports
dnalborczyk Jan 19, 2022
fa4c5b7
nits
dnalborczyk Jan 19, 2022
4a5fa73
fix: mark fsevents as external
dnalborczyk Jan 20, 2022
f413cb8
fix: set interop to default
dnalborczyk Jan 20, 2022
2ef6c5b
Revert "fix: mark fsevents as external"
dnalborczyk Jan 20, 2022
668ef16
chore: bump chokidar
dnalborczyk Jan 20, 2022
36dc97b
simplify alias plugin params
dnalborczyk Jan 20, 2022
64c0d06
Revert "Revert "fix: mark fsevents as external""
dnalborczyk Jan 20, 2022
1a156d5
add bogus types for fsevents
dnalborczyk Jan 20, 2022
46fc2e8
exclude fsevents from import linting
dnalborczyk Jan 20, 2022
b1e7f71
add back comment
dnalborczyk Jan 21, 2022
7bf3aab
bump typescript
dnalborczyk Jan 21, 2022
f58a3f7
re-use env var
dnalborczyk Jan 21, 2022
68f992d
Merge branch 'master' into rollup-config-external
lukastaegert Jan 21, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 4 additions & 1 deletion .eslintrc.js
View file
Open in desktop
Expand Up @@ -73,7 +73,10 @@ module.exports = {
'dot-notation': 'error',
'import/no-unresolved': [
'error',
{ ignore: ['package.json', 'is-reference', 'help.md', 'types'] }
{
// 'fsevents' is ony available on macOS, and not installed on linux/windows
ignore: ['fsevents', 'help.md', 'is-reference', 'package.json', 'types']
}
],
'import/order': ['error', { alphabetize: { order: 'asc' } }],
'no-constant-condition': ['error', { checkLoops: false }],
Expand Down
2 changes: 1 addition & 1 deletion build-plugins/conditional-fsevents-import.ts
View file
Open in desktop
@@ -1,5 +1,5 @@
import MagicString from 'magic-string';
import { Plugin } from 'rollup';
import type { Plugin } from 'rollup';

const FSEVENTS_REQUIRE = "require('fsevents')";
const REPLACEMENT = "require('../../../src/watch/fsevents-importer').getFsEvents()";
Expand Down
68 changes: 42 additions & 26 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions package.json
View file
Open in desktop
Expand Up @@ -75,7 +75,7 @@
"acorn-jsx": "^5.3.2",
"acorn-walk": "^8.2.0",
"buble": "^0.20.0",
"chokidar": "^3.5.2",
"chokidar": "^3.5.3",
"colorette": "^2.0.16",
"core-js": "^3.20.3",
"date-time": "^4.0.0",
Expand Down Expand Up @@ -114,7 +114,7 @@
"systemjs": "^6.11.0",
"terser": "^5.10.0",
"tslib": "^2.3.1",
"typescript": "^4.5.4",
"typescript": "^4.5.5",
"weak-napi": "^2.0.2",
"yargs-parser": "^20.2.9"
},
Expand Down
59 changes: 18 additions & 41 deletions rollup.config.ts
View file
Open in desktop
@@ -1,11 +1,12 @@
import { readFileSync } from 'fs';
import path from 'path';
import { resolve } from 'path';
import process from 'process';
import alias from '@rollup/plugin-alias';
import commonjs from '@rollup/plugin-commonjs';
import json from '@rollup/plugin-json';
import resolve from '@rollup/plugin-node-resolve';
import { nodeResolve } from '@rollup/plugin-node-resolve';
import typescript from '@rollup/plugin-typescript';
import { RollupOptions, WarningHandlerWithDefault } from 'rollup';
import type { RollupOptions, WarningHandlerWithDefault } from 'rollup';
import { string } from 'rollup-plugin-string';
import { terser } from 'rollup-plugin-terser';
import addCliEntry from './build-plugins/add-cli-entry';
Expand All @@ -14,7 +15,7 @@ import emitModulePackageFile from './build-plugins/emit-module-package-file';
import esmDynamicImport from './build-plugins/esm-dynamic-import';
import getLicenseHandler from './build-plugins/generate-license-file';
import replaceBrowserModules from './build-plugins/replace-browser-modules';
import pkg from './package.json';
import { version } from './package.json';

const commitHash = (function () {
try {
Expand All @@ -24,15 +25,12 @@ const commitHash = (function () {
}
})();

const now = new Date(
process.env.SOURCE_DATE_EPOCH
? 1000 * parseInt(process.env.SOURCE_DATE_EPOCH)
: new Date().getTime()
).toUTCString();
const { SOURCE_DATE_EPOCH } = process.env;
const now = new Date(SOURCE_DATE_EPOCH ? 1000 * +SOURCE_DATE_EPOCH : Date.now()).toUTCString();

const banner = `/*
@license
Rollup.js v${pkg.version}
Rollup.js v${version}
${now} - commit ${commitHash}

https://github.com/rollup/rollup
Expand All @@ -50,11 +48,11 @@ const onwarn: WarningHandlerWithDefault = warning => {
};

const moduleAliases = {
entries: [
{ find: 'help.md', replacement: path.resolve('cli/help.md') },
{ find: 'package.json', replacement: path.resolve('package.json') },
{ find: 'acorn', replacement: path.resolve('node_modules/acorn/dist/acorn.mjs') }
],
entries: {
acorn: resolve('node_modules/acorn/dist/acorn.mjs'),
'help.md': resolve('cli/help.md'),
'package.json': resolve('package.json')
},
resolve: ['.js', '.json', '.md']
};

Expand All @@ -66,7 +64,7 @@ const treeshake = {

const nodePlugins = [
alias(moduleAliases),
resolve(),
nodeResolve(),
json(),
conditionalFsEventsImport(),
string({ include: '**/*.md' }),
Expand All @@ -80,24 +78,8 @@ const nodePlugins = [
export default (command: Record<string, unknown>): RollupOptions | RollupOptions[] => {
const { collectLicenses, writeLicense } = getLicenseHandler();
const commonJSBuild: RollupOptions = {
// fsevents is a dependency of chokidar that cannot be bundled as it contains binary code
external: [
'buffer',
'@rollup/plugin-typescript',
'assert',
'crypto',
'events',
'fs',
'fsevents',
'module',
'os',
'path',
'perf_hooks',
'process',
'stream',
'url',
'util'
],
// 'fsevents' is a dependency of 'chokidar' that cannot be bundled as it contains binary code
external: ['fsevents'],
input: {
'loadConfigFile.js': 'cli/run/loadConfigFile.ts',
'rollup.js': 'src/node-entry.ts'
Copy link
Member

Choose a reason for hiding this comment

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

I was wondering why fsevents wasn't a problem but it turns out since you extracted the

const moduleName = 'fsevents'

constant in fsevents-importer.ts, the dynamic import can no longer be resolved by Rollup, which is why it does not need to be marked as external. Which is fine. But this also means, we can simplify the interop option below to interop: "default" and get rid of the function.

Copy link
Member

Choose a reason for hiding this comment

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

"defaultOnly" is just a stricter version of "default", so if "defaultOnly" works, "default" works as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was wondering why fsevents wasn't a problem but it turns out since you extracted the

const moduleName = 'fsevents'

constant in fsevents-importer.ts, the dynamic import can no longer be resolved by Rollup, which is why it does not need to be marked as external. Which is fine.

I changed the dynamic import back to fsevents, but it's failing on windows and linux as it seems rollup is trying to load fsevents nonetheless, which fails [since it's also not installed]: https://github.com/rollup/rollup/runs/4876116960?check_suite_focus=true is this a possible bug?

I'll revert for the time being.

But this also means, we can simplify the interop option below to interop: "default" and get rid of the function.

Done.

speaking of fsevents, also bumped chokidar.

Copy link
Member

Choose a reason for hiding this comment

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

I changed the dynamic import back to fsevents, but it's failing on windows and linux

And did you also add "fsevents" back to the external option? Because that is why it was here and why I originally though that externals should be an array containing just "fsevents".

Which I think is ok, i.e. to have your dependencies as "external". We could also employ a technique here used by many other plugins that just import their "package.json" file in their "rollup.config.js" and directly use package.dependencies as input for their external option. That way it "auto-updates" should we ever have additional dependencies.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, this is the TypeScript plugin complaining, not Rollup. Of course you would need to teach TypeScript about the missing dependency. That was the reason why we had

declare module 'fsevents' {
	export default {};
}

in declarations.dts, but you did not like that in the past and removed it. You could just add it back, maybe this time with a comment why we have it here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, this is the TypeScript plugin complaining, not Rollup.

DOH! 🤦 it got late 😆 ... and confusing, having fsevents installed on my mac with build and tests passing and having the failure only on windows/linux, where fsevents including type information is missing.

Of course you would need to teach TypeScript about the missing dependency. That was the reason why we had

declare module 'fsevents' {
	export default {};
}

with this PR: #4219 it's possible that the previously used [older?] typescript plugin wasn't quite working, but by bumping it to the newer version the typescript error came up:

from the PR description:

Description bumps rollup-plugin-typescript to latest @rollup/plugin-typescript. there appears to be a problem with the fsevents module and types under linux/windows, as those are optional deps and only installed on macOS. I had to use an any type to get around the typing problem. I'm not sure why and how it worked previously. 🤔

48a11b4

but changed it afterwards by just making the import parameter a variable.

in declarations.dts, but you did not like that in the past and removed it. You could just add it back, maybe this time with a comment why we have it here.

I checked the history, I [believe] I didn't remove those typings, but let me try if that would work as well on the mac when fsevents (plus typings) is actually installed.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I just checked one level of history and you are right 😉

Expand All @@ -114,12 +96,7 @@ export default (command: Record<string, unknown>): RollupOptions | RollupOptions
format: 'cjs',
freeze: false,
generatedCode: 'es2015',
interop: id => {
if (id === 'fsevents') {
return 'defaultOnly';
}
return 'default';
},
interop: 'default',
manualChunks: { rollup: ['src/node-entry.ts'] },
sourcemap: true
},
Expand Down Expand Up @@ -160,7 +137,7 @@ export default (command: Record<string, unknown>): RollupOptions | RollupOptions
plugins: [
replaceBrowserModules(),
alias(moduleAliases),
resolve({ browser: true }),
nodeResolve({ browser: true }),
json(),
commonjs(),
typescript(),
Expand Down
10 changes: 5 additions & 5 deletions src/watch/fsevents-importer.ts
View file
Open in desktop
@@ -1,18 +1,18 @@
let fsEvents: unknown;
import type FsEvents from 'fsevents';

let fsEvents: typeof FsEvents;
let fsEventsImportError: Error | undefined;

export async function loadFsEvents(): Promise<void> {
const moduleName = 'fsevents';

try {
({ default: fsEvents } = await import(moduleName));
({ default: fsEvents } = await import('fsevents'));
} catch (err: any) {
fsEventsImportError = err;
}
}

// A call to this function will be injected into the chokidar code
export function getFsEvents(): unknown {
export function getFsEvents(): typeof FsEvents {
if (fsEventsImportError) throw fsEventsImportError;
return fsEvents;
}
6 changes: 6 additions & 0 deletions typings/fsevents.d.ts
View file
Open in desktop
@@ -0,0 +1,6 @@
// 'fsevents' (which also has typings included) is an optional dependency installed on macOS,
// and not installed on linux/windows. this will provide (bogus) type information for
// linux/windows, and overwrite (replace) the types coming with the 'fsevents' module on macOS
declare module 'fsevents' {
export default {};
}