Build using rollup #8
Conversation
|
Hey @perrin4869 v busy lately! Am gonna merge asap okay?? 😄 |
|
@edoardo-bluframe no hurry, just a nice to have :) |
|
@edoardo-bluframe any updates? |
| @@ -1,3 +1,3 @@ | |||
| node_modules | |||
| lib/ | |||
| dist/ | |||
There was a problem hiding this comment.
Can we keep the name lib?
I actually really like it!
Is there a reason we are renaming to dist?
There was a problem hiding this comment.
Hm... well, usually I'd use lib for files which were transpiled using babel (usually I'd do that for modules meant to run only with node), and dist to distribute bundles output by rollup (or webpack).
Of course I can still change it if you prefer.
| @@ -2,27 +2,28 @@ | |||
| "name": "react-router-scroll-top", | |||
| "description": "An easy scroll to top component based on the React Router Scroll Restoration Example", | |||
| "version": "0.1.0", | |||
| "main": "index.js", | |||
| "main": "dist/react-router-scroll-top.cjs.js", | |||
| "build": "webpack", | ||
| "clean": "rm -fr lib/", | ||
| "build": "rollup -c", | ||
| "clean": "rm -fr dist/", |
| @@ -2,27 +2,28 @@ | |||
| "name": "react-router-scroll-top", | |||
| "description": "An easy scroll to top component based on the React Router Scroll Restoration Example", | |||
| "version": "0.1.0", | |||
| "main": "index.js", | |||
| "main": "dist/react-router-scroll-top.cjs.js", | |||
| "module": "dist/react-router-scroll-top.esm.js", | |||
| "prepublish": "yarn test && yarn run clean && yarn run build", | ||
| "test": "eslint src/ && flow && jest --silent" | ||
| }, | ||
| "devDependencies": { | ||
| "babel-core": "^6.26.0", | ||
| "@babel/core": "^7.1.2", |
There was a problem hiding this comment.
Love the upgrade to Babel 7!
| export default [{ | ||
| external, | ||
| input, | ||
| output: { exports: "named", file: `dist/${pkg.name}.cjs.js`, format: "cjs" }, |
| }, { | ||
| external, | ||
| input, | ||
| output: { file: `dist/${pkg.name}.esm.js`, format: "esm" }, |
| }, { | ||
| external, | ||
| input, | ||
| output: { exports: "named", file: `dist/${pkg.name}.umd.js`, format: "umd", globals, name }, |
src/index.js
Outdated
| @@ -1,7 +1,7 @@ | |||
| // @flow | |||
|
|
|||
| import { type Element } from "react" | |||
| import ScrollToTop from "./ScrollToTop" | |||
| import ScrollToTop from "./ScrollToTop.jsx" | |||
There was a problem hiding this comment.
Shall we just rename that file to ScrollToTop.js and leave this as import ScrolToTop from "./ScrollToTop"?
There was a problem hiding this comment.
It is usually good practice to name files with jsx in them jsx. I'd rather introduce rollup-plugin-node-resolve and add jsx as an extension, if we want to remove the .jsx from the import.
There was a problem hiding this comment.
Wasn't that the case till about 2 years ago and now we have all moved on to just .js and took it as a community-wide standard?
There was a problem hiding this comment.
Oh yeah looked into it and you are right :)
I've always stuck with airbnb's styling guide and they still insist on using jsx as a file extension, but since then, the React developers did move away from it. Updated the PR to reflect this
Here is a PR review! Enjoy okay?? 😄 |
|
Oh and @perrin4869 can we target |
| @@ -1,3 +1,3 @@ | |||
| node_modules | |||
| lib/ | |||
| dist/ | |||
There was a problem hiding this comment.
Hm... well, usually I'd use lib for files which were transpiled using babel (usually I'd do that for modules meant to run only with node), and dist to distribute bundles output by rollup (or webpack).
Of course I can still change it if you prefer.
src/index.js
Outdated
| @@ -1,7 +1,7 @@ | |||
| // @flow | |||
|
|
|||
| import { type Element } from "react" | |||
| import ScrollToTop from "./ScrollToTop" | |||
| import ScrollToTop from "./ScrollToTop.jsx" | |||
There was a problem hiding this comment.
It is usually good practice to name files with jsx in them jsx. I'd rather introduce rollup-plugin-node-resolve and add jsx as an extension, if we want to remove the .jsx from the import.
|
Maybe version |
|
rebased onto development |
|
Hm... I'm not very well acquainted with jest. Could you help me fix the test errors? Also, the next version should probably support |
|
@perrin4869 Hey I just tried the build from develop and it throws an error when I import it in my app Can you check that it works when you import |
|
Are you able to try that and see that you get that error? |
|
Okay - just investigated Our new version built with The version built with webpack works 100% with both Are you able to look into that @perrin4869 ? |
|
@edoardo-bluframe my guess would be multiple instances of |
|
Also, rollup is producing 3 different builds: |
|
Hey @perrin4869 I am just using Does that help?? 😄 |
|
remix-run/react-router#6755 |
(Better) alternative to #6
Builds 3 different modules,
cjs,umdandesm.