Skip to content
This repository has been archived by the owner on Dec 16, 2021. It is now read-only.

Added a rollup config section to the readme #398

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ansballard
Copy link

#397

Probably needs to be trimmed down, included everything needed to run the example below the configs.

I also referenced the es file instead of the umd one. I figured if you're using rollup, you probably want to prefer es when available.

If it were to be trimmed down to match browserify and webpack, I guess it would only need the alias sections, but I don't know how obvious it would be to add the rest of the plugins to get the example running (which this does).

…mmed down, included everything needed to run the example below the configs
README.md Outdated
resolve({
jsnext: true // preact
}),
alias({
Copy link
Member

Choose a reason for hiding this comment

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

Unrelated to this docs PR, but do you think it might be useful for preact-compat to include an export that auto-generates this?

import alias from 'rollup-plugin-alias';
import aliases from 'preact-compat/aliases';

export default {
  // ...
  plugins: [
    alias(aliases)
  ]
};

Copy link
Author

@ansballard ansballard Jun 21, 2017

Choose a reason for hiding this comment

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

oh, that's a cool idea. Although the function shorthand may confuse users that want to use more than just the preact aliases, since they would need to be a little more verbose and use destructuring with the regular object syntax.

Oh nevermind, they could just ... the args going in, I should know not to comment before the caffeine kicks in. Seems good to me!

@developit
Copy link
Member

It might be worth dropping all these how-to sections into <details> so they are collapsed by default?

<details>
 <summary>Summary Goes Here</summary>
 ...this is hidden, collapsable content...
</details>

README.md Outdated
"create-react-class": "node_modules/preact-compat/lib/create-react-class.js"
}),
commonjs(), // prop-types
replace({
Copy link

@crccheck crccheck Jun 30, 2017

Choose a reason for hiding this comment

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

If it were to be trimmed down...

The replace step seem unrelated to the intent of just using rollup + preact. As a newb, I'd prefer the smallest example that works. I'd prefer if this was trimmed

@ghost
Copy link

ghost commented Jul 19, 2017

Hmm this doesn't work for me. I get
Could not resolve '../Engine/node_modules/preact-compat/dist/preact-compat.es.js' from commonjs-proxy:../Engine/node_modules/preact-compat/dist/preact-compat.es.js ... tried switching up the order and stuff but doesnt seem to help

@ansballard
Copy link
Author

@cl4ws0n oh, those would probably need a path.resolve(__dirname, "node_modules", etc), looks like you're running out of a subdirectory or something. Sorry I dropped the ball on this, will try to get back and make a working stripped down version at some point.

- Removed all plugins except alias
- added path.resolve to aliases
@ansballard
Copy link
Author

Trimmed out everything but the alias plugin, and added path.resolve since it seems the string literal doesn't always work. I did not add the <details> sections, since it's a little out of scope and would probably fit better int a separate pull request, same with the preact-compat/aliases export.

@developit
Copy link
Member

Do you want to wait on this one until we get /aliases in?

@ansballard
Copy link
Author

Works for me, I'll try to remember to reference this issue in the pull request so it's easy to see when this can be updated.

@Ramblurr
Copy link

This might not be the right place to ask, but it would be nice to know the full setup for preact + babel + eslint + rollup

@developit
Copy link
Member

I opened #458 for the preact-compat/aliases entry.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants