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

Rollup Error: Could not resolve '../dom' from src/preact/src/vdom/diff.js #792

Closed
creationix opened this issue Aug 3, 2017 · 8 comments · Fixed by #796
Closed

Rollup Error: Could not resolve '../dom' from src/preact/src/vdom/diff.js #792

creationix opened this issue Aug 3, 2017 · 8 comments · Fixed by #796

Comments

@creationix
Copy link

I'm trying to use preact with rollup, but no babel. I hit a small snag that should be easy to fix.

I have this repo as a submodule directly in my src folder and am trying to use it like this:

/* eslint-env browser */
import { Component, h, render } from './preact/src/preact.js'

/** Example classful component */
class App extends Component {
  componentDidMount () {
    this.setState({ message: 'Hello!' })
  }
  render (props, state) {
    return (
      h('div', {id: 'app'},
        h(Header, { message: state.message }),
        h(Main)
      )
    )
  }
}

/** Components can just be pure functions */
const Header = (props) => {
  return h('header', null,
    h('h1', null, 'App'),
    props.message && h('h2', null, props.message)
  )
}

/** Instead of JSX, use: h(type, props, ...children) */
class Main extends Component {
  render () {
    const items = [1, 2, 3, 4, 5].map((item) => (
      h('li', {id: item}, 'Item ' + item)
    ))
    return (
      h('main', null,
        h('ul', null, items)
      )
    )
  }
}

render(h(App), document.body)

The problem is src/vdom/component.js and src/vdom/diff.js both import dom as simply ../dom, but they need to be ../dom/index because rollup's default search path doesn't support auto index on folders.

Once I tweak those two files, it seems to bundle just fine.

@creationix creationix changed the title Error: Could not resolve '../dom' from src/preact/src/vdom/diff.js Rollup Error: Could not resolve '../dom' from src/preact/src/vdom/diff.js Aug 3, 2017
@pl12133
Copy link
Contributor

pl12133 commented Aug 5, 2017

Could you please post your rollup.config.js?

Are you using rollup-plugin-node-resolve? Thanks

@creationix
Copy link
Author

It's a pretty simply rollup config:

export default {
  entry: 'src/main.js',
  dest: 'build/main.js',
  format: 'iife',
  plugins: [],
  sourceMap: true
}

I'm not using the node resolve plugin. Like I said, I'm including this repo as a submodule directly in my src folder.

@pl12133
Copy link
Contributor

pl12133 commented Aug 5, 2017

Thanks for posting your config, it looks like you may be experiencing rollup/rollup#346.

Would it be acceptable to add rollup-plugin-node-resolve, or would you suggest preact avoid Node/NPM specific behaviors?

@creationix
Copy link
Author

creationix commented Aug 6, 2017

I'd rather avoid needing a plugin for such a tiny thing. If you just add /index to your two requires, it will still work with node and other workflows, but also work with rollup out of the box without any plugins.

It's up to you, but I think such a small and simple change on your end would simplify consuming the library for me quite a bit.

slaskis added a commit to slaskis/preact that referenced this issue Aug 6, 2017
@pl12133
Copy link
Contributor

pl12133 commented Aug 7, 2017

I don't really have a preference here, but this solution is not very "small and simple" if you consider the hundreds of preact components on npm which depend on the Node path resolution strategy.

If this helps your with use case that is great, but I suppose I am not understanding the aversion to the use of rollup plugins when this problem is very easily solved by rollup plugins.

@creationix
Copy link
Author

Well, I checked and the plugin is much lighter-weight than I expected (npm dependencies tend to be giant).

$ npm install --save-dev rollup-plugin-node-resolve
$ du -sh node_modules/
668K	node_modules/
$ tree node_modules/
...
103 directories, 162 files

My project doesn't have a node_modules yet. Yes I personally have an aversion to including hundreds of files and over half a megabyte of code, having to manage node_modules in my .gitignore as well as track package.json and package-lock.json, and adding a build dependency to npm in my flow just to avoid adding 14 bytes to upstream preact.

I understand I'm fairly unique and most JS developers already use npm and much larger node_modules trees already and barely notice the cost of adding this plugin.

You also bring up a very good point about third-party components that depend on the node resolution strategy. Again I'm a unique case and am not consuming any existing components and so am not affected by this. Even the 14 bytes added to preact won't affect me because I'm using rollup and it erases all the import and exports statements by just inlining everything together.

The choice is yours, I respect your preferences as the maintainer. Just a humble request to help simplify for my fairly odd workflow.

@pl12133
Copy link
Contributor

pl12133 commented Aug 7, 2017

Thanks for explaining the use case. I understand the desire to keep the dependency tree shallow and easy to understand, and since rollup flattens the files this wouldn't have an effect on transpiled file size anyway. My last concern would be that future contributions would have to follow this pattern to avoid breaking changes in libraries such as yours which rely on the explicit import of /index; so we may need to include some documentation on that.

Since it doesn't affect performance I wouldn't mind seeing this merged, but I can't really speak with authority on whether it's worth the change. Either way thank you for explaining the details of your use case.

@developit
Copy link
Member

I'm happy to make the change - one of the index files is due for renaming anyway.

developit pushed a commit that referenced this issue Aug 20, 2017
* Changed ../dom -> ../dom/index 

Should take care of #792

* Changed ../dom -> ../dom/index
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 a pull request may close this issue.

3 participants