Skip to content

feat(rollup): simplify external #80

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

Merged
kentcdodds merged 1 commit into from
Dec 13, 2018
Merged

feat(rollup): simplify external #80

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 13 additions & 2 deletions src/config/rollup.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,8 +88,19 @@ if (isPreact) {
}

const externalPattern = new RegExp(`^(${external.join('|')})($|/)`)
const externalPredicate =
external.length === 0 ? () => false : id => externalPattern.test(id)

function externalPredicate(id) {
const isDep = externalPattern.test(id)
if (umd) {
// for UMD, we want to bundle all non-peer deps
return isDep
}
// for esm/cjs we want to make all node_modules external
// TODO: support bundledDependencies if someone needs it ever...
const isNodeModule = id.includes('node_modules')
Copy link
Contributor

@Andarist Andarist Dec 12, 2018

Choose a reason for hiding this comment

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

i'd make it more strict with RegExp /\/node_modules\//, u'd have to check if id is normalized here, if not u'd have to construct the regexp based on path.sep (to support windows)

const isRelative = id.startsWith('.')
return isDep || (!isRelative && !path.isAbsolute(id)) || isNodeModule
Copy link
Contributor

@Andarist Andarist Dec 12, 2018

Choose a reason for hiding this comment

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

(!isRelative && !path.isAbsolute(id))

what cases are covered by this which are not covered by isDep || isNodeModule?

Copy link
Owner Author

@kentcdodds kentcdodds Dec 12, 2018

Choose a reason for hiding this comment

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

Yeah, it's strange, but this would be called with '@emotion/styled-base' (not listed as a dep because it's added via a babel plugin). So I also need to factor in whether it's relative.

But I can't JUST do relative either because for each mistake it's called with both the source string id and the full path to the resolved module.

Copy link
Contributor

@Andarist Andarist Dec 12, 2018

Choose a reason for hiding this comment

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

but arent full paths already covered (kinda) by isNodeModule check?

Copy link
Owner Author

@kentcdodds kentcdodds Dec 12, 2018

Choose a reason for hiding this comment

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

Full paths are, but not non-relative paths. It's weird I know, but I spent a few hours on this in a project that uses a similar config (paypal-scripts) and this was the only way I found to fix it 🤷‍♂️

Copy link
Contributor

@Andarist Andarist Dec 12, 2018

Choose a reason for hiding this comment

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

hm, maybe its brain fart but I still dont get it 😅 non-relative paths can be either:

  • package name
  • absolute path (which should include node_modules)
  • alias (which is not covered, and arguably cant be, here)

am i missing something? 🤔

Copy link
Owner Author

@kentcdodds kentcdodds Dec 12, 2018

Choose a reason for hiding this comment

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

It's kinda hard to explain. All I can say is that if I didn't have the relative and absolute logic in here, then weird things would happen :/

Copy link
Contributor

@Andarist Andarist Dec 13, 2018

Choose a reason for hiding this comment

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

it shouldnt hurt, so im not opposed to it, was rather just curious why this is needed

APPROVED! 👍

}

const filename = [
pkg.name,
Expand Down