App skeleton #1

jakearchibald · 2018-03-26T10:00:59Z

No description provided.

…-skeleton

…ml) to webpack config.

surma

Okay, so we got together and went through the code. We just commented on everything that we didn’t understand — that doesn’t mean it’s wrong.

@developit please take a look and humor our ignorance. We kept the comments quite terse because we wanted to keep going while we are all 3 in the same physical room.

But, since this is the basis for the entire project, we just wanna make sure we all understand it and feel comfortable working with it.

surma · 2018-03-26T10:38:38Z

.babelrc

+			{
+				"loose": true,
+				"uglify": true,
+				"modules": "commonjs",


What is this for? Isn’t webpack taking care of all module-y stuff?

Yes - this was here for when I was trying to do prerendering without bundling via Webpack, but I've abandoned that attempt.

remove "modules" option

surma · 2018-03-26T10:43:08Z

webpack.config.js

@@ -0,0 +1,245 @@
+let fs = require('fs');


We should be using const here.

surma · 2018-03-26T10:43:51Z

config/watch-timestamps-plugin.js

+
+module.exports = WatchTimestampsPlugin;
+
+function WatchTimestampsPlugin(patterns) {


Can we use real classes here?

Sure, they should be Node 4+.

Use classes

I think we are all on Node 8, no? We should use all the goodness — our build system is not gonna turn into a library.

surma · 2018-03-26T10:45:02Z

config/watch-timestamps-plugin.js

@@ -0,0 +1,23 @@
+let fs = require('fs');
+
+module.exports = WatchTimestampsPlugin;


I’d really like a comment to explain what this does 😅

I'll find the GH issue I retrofitted this from.

surma · 2018-03-26T10:45:40Z

config/watch-timestamps-plugin.js

+WatchTimestampsPlugin.prototype.apply = function (compiler) {
+	compiler.plugin('watch-run', (watch, callback) => {
+		let patterns = this.patterns;
+		let timestamps = watch.fileTimestamps || watch.compiler.fileTimestamps;


What are these two alternatives?

Webpack 3 and prior vs Webpack 4+. Also I believe it may vary depending on whether the plugin is invoked in a child compiler instance.

Same as above — we decide what WebPack version we use.

Agreed - likely this came from before I committed to v4.

Remove webpack 3 & prior stuff from WatchTimestampsPlugin

surma · 2018-03-26T11:38:13Z

src/components/app/index.tsx

+			files: [fileObj]
+		});
+
+		let fr = new FileReader();


Jake says you should new new Response() here (example)

I think we should move this into a util modules as loadFileAsBlob() or something.

For some reason I never remember that Response accepts a Blob.

~~Move loadFileAsBlob() to a util~~

Use new Response(blob).text()

Ended up being as simple as new Response(file).text(), not worth a util.

surma · 2018-03-26T11:40:51Z

src/components/app/index.tsx

+
+	enableDrawer = false;
+
+	openDrawer = updater(this, 'showDrawer', true);


These should be proper methods. We can use TypeScript decorators to tie in stuff like updater()

I'll try making a decorator. Personally I've been gravitating towards using a "store" to house data + mutators/actions. I also threw some ideas around with @davideast last week for simplifying state+selectors, and he published something called selectrify.

I've just turned these into methods for now.

surma · 2018-03-26T11:42:39Z

src/components/app/index.tsx

+	render({ url }, { showDrawer, showFab, files }) {
+		if (showDrawer) this.enableDrawer = true;
+
+		if (showFab===true) showFab = files.length>0;


if(showFab)? :D

surma · 2018-03-26T11:44:22Z

src/components/app/index.tsx

+			<div id="app" class={style.app}>
+				<Fab showing={showFab} />
+
+				<Header toggleDrawer={this.toggleDrawer} loadFile={this.loadFile} />


Is toggleDrawe an event handler? If so, we probably wanna call it onToggleDrawer or something?

(same for the other eventhandler-y stuff)

I assume you mean the left-hand side? If so I agree!

onToggleDrawer

surma · 2018-03-26T11:48:52Z

src/components/app/index.tsx

+				</div>
+
+				{/* This ends up in the body when prerendered, which makes it load async. */}
+				<link rel="stylesheet" href="https://fonts.googleapis.com/icon?family=Material+Icons" />


It doesn’t make it load async, it just renders what is before it, and even that is not very widely implemented. We should figure out how we handle fonts in this project.

Agreed, plus without prerendering my comment is inaccurate anyway (though I believe stylesheets added by script are async, unlike those dumped into body).

move it

…nything vague

…ict mode.

… up files, so we need to explicitly feed it a files list via `tsconfig.json`.

developit · 2018-03-30T01:33:20Z

Alrighty @surma - that should be all the feedback acted on!

…in order to defer binding until there is an instance to bind to, the simpler implementation actually bound methods to the prototype itself.

… config, drop threaded type checking (couldn't get it working)

surma · 2018-04-09T13:52:18Z

config/prerender-loader.js

+
+    let html = dom.serialize();
+    callback(null, html);
+    // return html = `module.exports = ${JSON.stringify(html)}`;


Can these comments all go?

surma · 2018-04-09T13:54:49Z

src/components/app/style.scss

+  .content {
+    flex: 1 1 auto;
+    contain: size layout style;
+    overflow: auto;


Do we really wanna scroll on content? It usually ends up causing problems. I’d prefer scrolling on body and having sidebar/topbar position: fixed.

I think we've got too much layout in the PR. If it's either-or, I'd rather we landed something that simply demonstrated that CSS could be included, and didn't otherwise worry about layout.

Layout is something we can plan & do in another PR.

surma · 2018-04-09T13:55:09Z

src/components/drawer/index.tsx

+import MdlDrawer from 'preact-material-components-drawer';
+import 'preact-material-components/Drawer/style.css';
+import List from 'preact-material-components/List';
+// import 'preact-material-components/List/style.css';


surma · 2018-04-09T13:55:22Z

src/components/app/index.tsx

+import { When, bind } from '../../lib/util';
+import Fab from '../fab';
+import Header from '../header';
+// import Drawer from 'async!../drawer';


surma · 2018-04-09T13:55:33Z

config/prerender.js

+  global.history = {};
+  global.location = { href: url, pathname: url };
+
+  // let app = require('app-entry-point');


surma · 2018-04-09T13:56:51Z

src/components/home/style.scss

@@ -0,0 +1,20 @@
+@import '~style/helpers.scss';
+
+// :global {


surma · 2018-04-09T13:57:06Z

src/index.html

+	</head>
+	<body>
+		<div id="app" prerender></div>
+		<!-- <link rel="stylesheet" href="https://fonts.googleapis.com/icon?family=Material+Icons" /> -->


surma · 2018-04-09T13:58:22Z

src/style/index.scss

@@ -0,0 +1,27 @@
+// @import './material-icons.scss';


surma · 2018-04-09T13:58:47Z

src/style/material-icons.scss

+  font-weight: 400;
+  src: local('Material Icons'),
+    local('MaterialIcons-Regular'),
+    url(https://example.com/MaterialIcons-Regular.woff2) format('woff2'),


Quotes like in index.scss?

I don't think this PR should contain anything like icon fonts.

surma · 2018-04-09T14:00:13Z

src/style/reset.scss

+  user-select: none;
+  -webkit-user-select: none;
+  user-drag: none;
+  -webkit-user-drag: none;


Should we use something like autoprefixer?

I don't think we should decide that as part of this PR.

jakearchibald · 2018-04-09T15:51:34Z

In general, I think this PR is trying to do too much.

We don't really need large parts of the app complete, we just need a build system that supports a mix of assets, preact & web components.

I worry that by bringing in things like icon fonts, and making layout decisions, is just delaying this being able to land.

Any chance of keeping it to the basics? Then we can start planning and dividing up the rest of the work.

surma · 2018-04-09T15:56:28Z

I’m absolutely fine with punting on the specifics and picking those back up when they get touched.

Mostly want the dead code gone, tho.

jakearchibald · 2018-04-15T13:59:37Z

src/lib/util.ts

+
+  render({ value, children = [] }: WhenProps, { ready }: WhenState) {
+    let child = children[0];
+    if (value && !ready) this.setState({ ready: true });


What's the benefit of ready here? Can't you just test value?

would create an infinite loop if the component loaded was undefined

jakearchibald · 2018-04-15T14:00:52Z

src/index.html

+		<link rel="manifest" href="/manifest.json">
+	</head>
+	<body>
+		<div id="app" prerender></div>


Why invent an attribute here rather than use a class name (or a data attribute)?

fixed in subsequent branch (prerender)

developit · 2018-04-17T17:44:37Z

note: rest of the stuff started here is getting its own PR's.

App skeleton

developit added 6 commits March 9, 2018 13:24

interim commit

60eafdc

Webpack@4 + mini-css-extract-plugin

619080f

Adds an app skeleton. Still needs some work.

e691ec9

Merge branch 'master' of github.com:GoogleChromeLabs/squoosh into app…

fcb8ba6

…-skeleton

Add progress bar and bundle analyzer (http://localhost:8080/report.ht…

49c67ba

…ml) to webpack config.

Convert remaining components to TypeScript.

9fa67c2

jakearchibald requested review from surma and kosamari March 26, 2018 10:01

surma requested changes Mar 26, 2018

View reviewed changes

developit added 20 commits March 27, 2018 20:34

Remove modules babelrc option

cd4fd07

let -> const

465d043

Use classes for webpack plugins.

ccf7155

Add explanation and credit to WatchTimestampsPlugin.

7fca9ac

Remove support for Webpack 3.x and prior from WatchTimestampsPlugin.

2ff3de5

Object.keys().forEach() --> for..of

e501fff

Get rid of DeprecationWarning output filter

a5e2952

Error out if required (json) configs are missing/corrupted.

720a11c

infer NODE_ENV from isProd

83a0bd1

Drop custom preact alias

e0a10cf

Remove commented-out Webpack config and add explanatory comments to a…

e347cbe

…nything vague

add note about filter(Boolean)

9977e5b

Use strict TypeScript, enable TSLint, update all types to work in str…

5e6500d

…ict mode.

tabs -> spaces

76ceea0

Dependency updates.

df8d5cf

if (showFab)

17e9668

Use new Response(file).text() to obtain a binary string

c741f24

toggleDrawer() -> onToggleDrawer()

048524c

I blame myself for this @surma

ac3eaee

fork-ts-checker-notifier-webpack-plugin doesn't use Webpack to look…

ae1d323

… up files, so we need to explicitly feed it a files list via `tsconfig.json`.

developit added 7 commits March 29, 2018 23:11

Add missing declare (fixes type errors on rebuild)

473a7d4

Fix missing header

5cfeefa

Our simplified @bind decorator didn't work :( The getter is needed …

8f452dd

…in order to defer binding until there is an instance to bind to, the simpler implementation actually bound methods to the prototype itself.

Make @bind the same as Surma's implementation

fc84c81

Fix broken sourcemaps in the TypeScript+Babel setup, clean up webpack…

1dd0366

… config, drop threaded type checking (couldn't get it working)

In development, persist application state across hot reloads.

d023263

Load images as both binary strings and blobs.

fbcd160

surma requested changes Apr 9, 2018

View reviewed changes

jakearchibald commented Apr 15, 2018

View reviewed changes

developit mentioned this pull request Apr 17, 2018

async/await for critters #16

Closed

developit merged commit b911e96 into master Apr 17, 2018

jakearchibald deleted the app-skeleton branch July 16, 2018 16:58

alisaitbilgi pushed a commit to alisaitbilgi/squoosh that referenced this pull request Feb 19, 2019

Merge pull request GoogleChromeLabs#1 from GoogleChromeLabs/app-skeleton

5cf07a8

App skeleton

HereChen mentioned this pull request Jul 27, 2020

codecs/mozjpeg_enc/example.html is not avalible (mozjpeg_enc.js generated by emscripten 1.39.20) #783

Closed


		module.exports = WatchTimestampsPlugin;

		function WatchTimestampsPlugin(patterns) {

		@@ -0,0 +1,23 @@
		let fs = require('fs');

		module.exports = WatchTimestampsPlugin;


		enableDrawer = false;

		openDrawer = updater(this, 'showDrawer', true);

App skeleton #1

App skeleton #1

Conversation

jakearchibald commented Mar 26, 2018

surma left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

developit Mar 26, 2018 • edited Loading

Choose a reason for hiding this comment

surma Mar 26, 2018 • edited by developit Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

developit Mar 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

developit Mar 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

developit Mar 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

developit Mar 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

surma Mar 26, 2018 • edited by developit Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

developit Mar 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

developit Mar 26, 2018 • edited Loading

Choose a reason for hiding this comment

developit commented Mar 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakearchibald commented Apr 9, 2018

surma commented Apr 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

developit commented Apr 17, 2018

developit Mar 26, 2018 •

edited

Loading

surma Mar 26, 2018 •

edited by developit

Loading

developit Mar 26, 2018 •

edited

Loading

developit Mar 26, 2018 •

edited

Loading

developit Mar 26, 2018 •

edited

Loading

developit Mar 26, 2018 •

edited

Loading

surma Mar 26, 2018 •

edited by developit

Loading

developit Mar 26, 2018 •

edited

Loading

developit Mar 26, 2018 •

edited

Loading