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

Suggest changes to how module is published #4

Merged
merged 1 commit into from
Dec 3, 2015
Merged

Suggest changes to how module is published #4

merged 1 commit into from
Dec 3, 2015

Conversation

sheepsteak
Copy link
Contributor

Apologies if this isn't what you want at all but I thought I'd try and tackle #3. Once I started I realised it might be useful to change how the module is delivered to npm, in a way similar to React itself:

  • A normal version straight from Babel is now given when require('preact') is used (from the lib directory) for development. It will also be multiple files in lib after Factor into modules #3 is done.
  • A single file UMD version (preact.js), source map (preact.js.map) and a single file UMD minified version (preact.min.js) is available in dist. It's also pushed to npm so that it's possible to do require('preact/dist/preact.min') when in production mode.

@developit
Copy link
Member

@sheepsteak wow, this is awesome. Any chance you give some stats on changes to the output filesize?
Also (and I really should have done a better job of indicating this in the ticket) I was thinking since this lib is ES6 and has no dependencies it would make the most sense to use Rollup - it achieves the smallest output filesize when bundling, and avoids wrapping modules in closures, which is good for performance.
Thoughts?

@sheepsteak
Copy link
Contributor Author

So npm run size is still running on the minified version and it's coming out at:
gzip size: 3749 / 3.75 kB

I'd never actually heard of Rollup before but I've just been having a look now. Were you thinking of approaching it like they say here? - https://github.com/rollup/rollup/wiki/jsnext:main

So we'd add 'jsnext:main': 'src/preact' to package.json for someone using Rollup downstream and also use Rollup to build a UMD bundle for everyone else?

@developit
Copy link
Member

I would be fine adding jsnext:main since its totally opt-in, though I know there was some discussion amongst Rich and the other contributors around the future of that property and exporting ES2015 code in NPM.

For now I think that's likely the best setup possible for Preact.

Do you think it makes sense to try rollup in a second PR? (Yours or mine, doesn't matter to me) - I might have an example rollup config from the last project I used it on we can borrow.

@developit
Copy link
Member

Found the setup I mentioned. Seems like it would work:

{
  "name": "preact",
  "amdName": "preact",
  "version": "3.0.0",
  "main": "dist/preact.js",
  "jsnext:main": "src/index.js",
  "minified:main": "dist/preact.min.js",
  "scripts": {
    "build": "npm-run-all rollup minify size docs",
    "rollup": "rollup -c rollup.config.js -m -f umd -n $npm_package_amdName src/index.js -o $npm_package_main",
    "minify": "uglifyjs $npm_package_main -cm -o $npm_package_minified_main -p relative --in-source-map ${npm_package_main}.map --source-map ${npm_package_minified_main}.map",
    "size": "size=$(gzip-size $npm_package_main) && echo \"gzip size: $size / $(pretty-bytes $size)\"",
    "release": "npm run build -s && git commit -am $npm_package_version && git tag $npm_package_version && git push && git push --tags && npm publish"
  },
  "devDependencies": {
    "babel-core": "^5.8.30",
    "gzip-size-cli": "^1.0.0",
    "pretty-bytes": "^2.0.1",
    "rollup": "^0.20.2",
    "rollup-plugin-babel": "^1.0.0",
    "uglify-js": "^2.5.0"
  }
}

I agree regarding not shipping a minified build as the default. I also wonder if there is really a reason to ship a UMD build by default, since NPM is technically for commonjs. It would actually be nice to ship the comments-removed babel build with an inline sourcemap, since that's the only thing Webpack supports out-of-the-box using source-map-loader. Seem decent enough?

@sheepsteak
Copy link
Contributor Author

Looks good! Yeah, I'm not sure about UMD over npm either. Want me to close this PR?

@developit
Copy link
Member

Up to you - I can try to get a PR for Rollup support submitted today and include you on it so you can take a look. I'll be using your setup as a guide, definitely switching to a gitignore'd dist as in yours.

@sheepsteak
Copy link
Contributor Author

I thought I'd have a go based on what you posted 🙊

Should I make it an inline source map though?

@developit
Copy link
Member

Just looking now. Not sure if you've run into this but I can't get source-map-loader to pull external maps when using npm link. Definitely not a huge priority though. I'll check this out locally and see how it goes 👯

@developit
Copy link
Member

@sheepsteak this looks perfect - only thing is the sourcemap path is incorrect, but that is a bug/feature of Rollup. I've opened Issue #344 there. For now, a fix would be to add sourceMapFile to rollup.config.js:

import babel from 'rollup-plugin-babel';
import path from 'path';
import fs from 'fs';

export default {
    sourceMapFile: path.resolve(JSON.parse(fs.readFileSync('./package.json')).main),
    plugins: [
        babel({
            sourceMap: true
        })
    ]
};

developit added a commit that referenced this pull request Dec 3, 2015
Suggest changes to how module is published
@developit developit merged commit a9ad805 into preactjs:master Dec 3, 2015
@sheepsteak
Copy link
Contributor Author

OK, still need me to do that? I see it's been merged now.

@developit
Copy link
Member

Doing it during the merge. Will commit & release in a sec :)

@sheepsteak
Copy link
Contributor Author

OK, thanks :D

@developit
Copy link
Member

Boom! released as 2.4.0 (and subsequently 2.4.1 :P).
Thanks muchly!

marvinhagemeister pushed a commit that referenced this pull request Mar 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants