Skip to content

replaces http-server with a live-reloading rollup dev server#39

Closed
telamonian wants to merge 2 commits intofinos:masterfrom
telamonian:add-rollup-dev-server
Closed

replaces http-server with a live-reloading rollup dev server#39
telamonian wants to merge 2 commits intofinos:masterfrom
telamonian:add-rollup-dev-server

Conversation

@telamonian
Copy link
Copy Markdown
Contributor

@telamonian telamonian commented Jun 8, 2020 •
edited
Loading

This PR:

  • cleans up the build and watch cmds in package.json
  • replaces all uses of http-server with a dev server that runs as part of a rollup build
  • the rollup watch build will now also launch a dev server. The dev server serves the examples, which will auto-reload on changes to anything in src/ (including the .less files)
  • the rollup watch build is now a "dev" build. The only real change is that it now skips the terser plugin, for easier debugging

@telamonian
Copy link
Copy Markdown
Contributor Author

telamonian commented Jun 8, 2020 •
edited
Loading

The only thing I haven't figured out is how to incorporate the .less sources into the watch build:

  • watch src/less/material.less and rebuild dist/css/material.css on changes
  • watch src/less/container.less and rebuild the regular-table umd bundle on changes

but that can wait for a later PR

@telamonian telamonian force-pushed the add-rollup-dev-server branch 2 times, most recently from ec57694 to 4710558 Compare June 8, 2020 12:16
@telamonian
Copy link
Copy Markdown
Contributor Author

telamonian commented Jun 8, 2020 •
edited
Loading

I figured out how to incorporate the .less files into rollup. It appears that rollup can't "just"
transpile the single material.less file, so it also outputs a dist/css/material.js file. This .js file is basically empty, and I set up the files paths in package.json to exclude it from the tarball, so this doesn't seem like a significant negative to me. On the plus side:

  • entire build now in single workflow/command
  • rollup watch will rebuild correctly on changes to the .less sources
  • optional source maps pointing back to original .less sources
  • regular-table.js bundle is sliiiightly smaller (to the tune of 20 bytes)

@telamonian telamonian force-pushed the add-rollup-dev-server branch 6 times, most recently from 63f9a5c to 2f8a97f Compare June 9, 2020 04:06
@telamonian telamonian closed this Jun 9, 2020
@telamonian telamonian reopened this Jun 9, 2020
@telamonian telamonian force-pushed the add-rollup-dev-server branch from 2f8a97f to 8994c7f Compare June 9, 2020 04:16
@telamonian
Copy link
Copy Markdown
Contributor Author

telamonian commented Jun 9, 2020

@texodus As requested, this PR now completely removes http-server in favor of the rollup dev server. This means that the jest-puppeteer tests now also run using the rollup dev build/server.

I ran into an issue on the CI (which annoyingly didn't show up locally) where jest was trying to run tests before the rollup dev server had finished its build/start-up cycle. I was able to resolve this by having jest wait on the resources in dist to show up before starting

@telamonian telamonian changed the title adds a live-reloading dev server to the rollup watch build replaces http-server with a live-reloading rollup dev server Jun 9, 2020
@telamonian telamonian added developement Related to perspective development, as opposed to developer usage enhancement Feature requests or improvements labels Jun 9, 2020
@texodus texodus mentioned this pull request Jun 12, 2020
Merged
texodus
texodus reviewed Jun 12, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@texodus texodus left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

I have played around with this branch and it's a nice QoL improvement for development - except rollup-plugin-serve is just worse than http-server. However, this change is not necessary to support live-reload, so let's just remove this part.

I've actually already done this on #50, as well as rebasing and fixing the other review comments below, so I'll close this PR in lieu of that.

Comment thread .travis.yml

# test production build
- yarn build
- yarn clean
Copy link
Copy Markdown
Member

@texodus texodus Jun 12, 2020

Choose a reason for hiding this comment

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

Really not fortunate that this builds twice ..

Comment thread rollup.config.js
extract: "material.css",
sourceMap: false,
minimize: true,
}),
Copy link
Copy Markdown
Member

@texodus texodus Jun 12, 2020

Choose a reason for hiding this comment

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

postcss is a good addition! It was not possible with the old babel-based build but is quite simple with rollup

Comment thread rollup.dev.config.js
import serve from "rollup-plugin-serve"
import sourcemaps from "rollup-plugin-sourcemaps";

export default commandLineArgs => {
Copy link
Copy Markdown
Member

@texodus texodus Jun 12, 2020

Choose a reason for hiding this comment

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

This file is 99% identical to rollup.config.js - it would be really helpful to not duplicate this as the project grows. Since this version is only called with --watch - why not merge these and simply dispatch the minimize and plugins changes based on this flag as you do --port

Comment thread rollup.dev.config.js
serve({
contentBase: [".", "examples"],
port
}),
Copy link
Copy Markdown
Member

@texodus texodus Jun 12, 2020

Choose a reason for hiding this comment

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

There is no nice way to say this - rollup-plugin-server is unconditionally worse than http-server :(

Comment thread src/js/scroll_panel.js
import {DEBUG, BROWSER_MAX_HEIGHT, DOUBLE_BUFFER_RECREATE, DOUBLE_BUFFER_ROW, DOUBLE_BUFFER_COLUMN} from "./constants";

import container_css from "../less/container.less";

Copy link
Copy Markdown
Member

@texodus texodus Jun 12, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This is great to remove but it leaves the vestigial code - the babel plugin and symbol in utils.js

@texodus
Copy link
Copy Markdown
Member

texodus commented Jun 12, 2020

Picked up in #50

@texodus texodus closed this Jun 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@texodus texodus texodus left review comments

Assignees

No one assigned

Labels

developement Related to perspective development, as opposed to developer usage enhancement Feature requests or improvements

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@telamonian @texodus