Skip to content

Commit

Permalink
fix(rollup): pass collected externals also for function externals
Browse files Browse the repository at this point in the history
close #39, see also: #38#issuecomment-549130017
  • Loading branch information
JounQin committed Nov 3, 2019
1 parent a49a40f commit a49bdf4
Show file tree
Hide file tree
Showing 2 changed files with 18 additions and 16 deletions.
31 changes: 16 additions & 15 deletions packages/rollup/src/config.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
// tslint:disable: no-big-function
import fs from 'fs'
import path from 'path'

Expand Down Expand Up @@ -139,7 +138,7 @@ export interface ConfigOptions {
exclude?: string[]
outputDir?: string
exports?: OutputOptions['exports']
externals?: string[] | ((id: string) => boolean)
externals?: string[] | ((id: string, collected?: string[]) => boolean)

This comment has been minimized.

Copy link
@tunnckoCore

tunnckoCore Nov 3, 2019

Allow a single string as external. With the globs & string regex support, it's enough to pass just a single string.

One more thing is to support both singular and the plural - options.external and options.externals. Then assign it to a constant called external (to match the real options as called by Rollup).

This comment has been minimized.

Copy link
@tunnckoCore

tunnckoCore Nov 3, 2019

Also, rename the current const external to collectedExternals to match more accurately - this part here: a49bdf4#diff-a81947f7a7c928c2669d4be612517d88R269-R276.

This also resolves a possible confusion to some reviewers down where it is

external(id) {
  if (externals === function) {
-    externals.call(this, id, external)
+    externals.call(this, id, collectedExternals)
  }
}

I'm just always about "clean(er) & intuitive code" :)

This comment has been minimized.

Copy link
@tunnckoCore

tunnckoCore Nov 3, 2019

Also need to include dependencies ini the collectedExternals, as commented.

globals?: StringMap
aliases?: StringMap | AliasOptions['entries']
copies?: StringMap | CopyOptions['targets'] | CopyOptions
Expand Down Expand Up @@ -313,18 +312,20 @@ ConfigOptions = {}): RollupOptions[] => {
globals,
exports,
},
external:
typeof externals === 'function'
? externals
: (id: string) =>
external.some(pkg => {
const pkgRegExp = tryRegExp(pkg)
return pkgRegExp instanceof RegExp
? pkgRegExp.test(id)
: isGlob(pkg)
? isMatch(id, pkg)
: id === pkg || id.startsWith(pkg + '/')
}),
external(id: string) {
if (typeof externals === 'function') {
return externals.call(this, id, external)

This comment has been minimized.

Copy link
@tunnckoCore

tunnckoCore Nov 3, 2019

Hoho. The this is cool one!

}
return external.some(pkg => {
const pkgRegExp = tryRegExp(pkg)
return pkgRegExp instanceof RegExp
? pkgRegExp.test(id)
: // TODO: should we drop glob support in favor of regexp?

This comment has been minimized.

Copy link
@tunnckoCore

tunnckoCore Nov 3, 2019

Definitely not. String regex is weird and hard, the pure awful example is Jest configuration.

isGlob(pkg)
? isMatch(id, pkg)
: id === pkg || id.startsWith(pkg + '/')
})
},
onwarn,
plugins: [
alias(aliasOptions),
Expand Down Expand Up @@ -361,7 +362,7 @@ ConfigOptions = {}): RollupOptions[] => {
postcss(postcssOpts),
].concat(
[
// __DEV__ will always be replaced while `process.env.NODE_ENV` will be preserved except on production
// __DEV__ and __PROD__ will always be replaced while `process.env.NODE_ENV` will be preserved except on production
define &&
replace(
prod
Expand Down
3 changes: 2 additions & 1 deletion packages/utils/src/browser.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import chalk from 'chalk'
import { execSync } from 'child_process'

import chalk from 'chalk'
import spawn from 'cross-spawn'
import open from 'open'

Expand Down

0 comments on commit a49bdf4

Please sign in to comment.