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

Add Parcel for bundling #55

Merged
merged 14 commits into from
Apr 27, 2020
Merged

Add Parcel for bundling #55

merged 14 commits into from
Apr 27, 2020

Conversation

amirhmk
Copy link
Collaborator

@amirhmk amirhmk commented Apr 20, 2020

Closes #50

After doing a bit of research and playing around, I've configured Parcel for the app and modified the structure where necessary. There is now a client folder which serves the frontend, and the public directory contains the html entry point.

The new structure looks like this:

client/
    components/
        data/js
    css/
    index.js
    main.js
dist/
models/
node_modules/
public/
    assets/
    index.html
package.json

I have been a little puzzled by this, but I am not too sure if the performance has improved as a result of this. Configuration was definitely pretty straightforward, and it was nice not having to write a bunch of web pack configurations. I think I have ported everything to be under this bundler, but I might have missed some functionalists too.

@Berkmann18 Do you have any early ideas why performance may not be at its best? I guess prod will be faster, but dev looks kind of odd.

Otherwise I can try looking into web pack as well and having a go at it.

TODO:

  • Update README

  • Test Performance

@amirhmk amirhmk requested a review from Berkmann18 April 20, 2020 00:59
public/dev.html Outdated Show resolved Hide resolved
client/components/notify.js Outdated Show resolved Hide resolved
@Berkmann18
Copy link
Collaborator

Once @misterpeddy is happy with the latest Lighthouse CI changes (re: documentation), I'll merge it and we'll be able to see how the changes perform here.
In the meantime, you can always test locally and see how much improvements it leads to.

@Berkmann18
Copy link
Collaborator

Berkmann18 commented Apr 20, 2020

After locally testing the built version (using npm run build && serve -d dist) and the dev version of index.html (npm run dev) it seems that there's no visible improvement compared to the master results.

Build Perf A11y BP SEO PWA Average
master 13 94 79 75 0 / 0 / 2 41.37%
npm run build && serve -d dist/ 13 94 79 75 0 / 0 / 2 41.37%
npm run dev 6 94 79 75 0 / 0 / 2 40.37%

(I wonder why running the dev site served by parcel leads to such a performance).

After digging into the performance result, the difference is intriguing:

Perf master build change
FCP 4.0s 4.4s +.4
SI 40.3s 10.4s -29.9
TTI 85.9s 158.4s +72.5
FMP 4.0s 4.4s +.4
FCI 85.6s 158.5s +72.9
MPFID 6,820ms 6,980ms +160

I suppose, the improvements from #48 will lead to better figures overall, but it seems like the minification and bundle import needs to be tweaked.

.env.train Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
client/main.js Outdated Show resolved Hide resolved
.stylelintignore Outdated Show resolved Hide resolved
@Berkmann18
Copy link
Collaborator

Berkmann18 commented Apr 25, 2020

Are you also getting UnhandledPromiseRejectionWarning errors?
(node:207204) UnhandledPromiseRejectionWarning: Error: Could not locate the bindings file. Tried:
 → /home/maxie/Projects/Others/hands-down/node_modules/deasync/build/deasync.node
 → /home/maxie/Projects/Others/hands-down/node_modules/deasync/build/Debug/deasync.node
 → /home/maxie/Projects/Others/hands-down/node_modules/deasync/build/Release/deasync.node
 → /home/maxie/Projects/Others/hands-down/node_modules/deasync/out/Debug/deasync.node
 → /home/maxie/Projects/Others/hands-down/node_modules/deasync/Debug/deasync.node
 → /home/maxie/Projects/Others/hands-down/node_modules/deasync/out/Release/deasync.node
 → /home/maxie/Projects/Others/hands-down/node_modules/deasync/Release/deasync.node
 → /home/maxie/Projects/Others/hands-down/node_modules/deasync/build/default/deasync.node
 → /home/maxie/Projects/Others/hands-down/node_modules/deasync/compiled/14.0.0/linux/x64/deasync.node
 → /home/maxie/Projects/Others/hands-down/node_modules/deasync/addon-build/release/install-root/deasync.node
 → /home/maxie/Projects/Others/hands-down/node_modules/deasync/addon-build/debug/install-root/deasync.node
 → /home/maxie/Projects/Others/hands-down/node_modules/deasync/addon-build/default/install-root/deasync.node
 → /home/maxie/Projects/Others/hands-down/node_modules/deasync/lib/binding/node-v83-linux-x64/deasync.node
    at bindings (/home/maxie/Projects/Others/hands-down/node_modules/bindings/bindings.js:126:9)
    at Object. (/home/maxie/Projects/Others/hands-down/node_modules/deasync/index.js:30:31)
    at Module._compile (/home/maxie/Projects/Others/hands-down/node_modules/v8-compile-cache/v8-compile-cache.js:194:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1205:10)
    at Module.load (internal/modules/cjs/loader.js:1034:32)
    at Function.Module._load (internal/modules/cjs/loader.js:923:14)
    at Module.require (internal/modules/cjs/loader.js:1074:19)
    at require (/home/maxie/Projects/Others/hands-down/node_modules/v8-compile-cache/v8-compile-cache.js:161:20)
    at Object. (/home/maxie/Projects/Others/hands-down/node_modules/parcel-bundler/src/utils/syncPromise.js:1:79)
    at Module._compile (/home/maxie/Projects/Others/hands-down/node_modules/v8-compile-cache/v8-compile-cache.js:194:30)
(Use `node --trace-warnings ...` to show where the warning was created)
(node:207204) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). To terminate the node process on unhandled promise rejection, use the CLI flag `--unhandled-rejections=strict` (see https://nodejs.org/api/cli.html#cli_unhandled_rejections_mode). (rejection id: 1)
(node:207204) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

@amirhmk
Copy link
Collaborator Author

amirhmk commented Apr 25, 2020

Are you also getting UnhandledPromiseRejectionWarning errors?

Hmm I don't get those actually, I do get a couple of warnings for DevTools not being able to load SourceMap. Does this pop up straight away or after a certain action? :)

@Berkmann18
Copy link
Collaborator

Berkmann18 commented Apr 25, 2020

Are you also getting UnhandledPromiseRejectionWarning errors?

Hmm I don't get those actually, I do get a couple of warnings for DevTools not being able to load SourceMap. Does this pop up straight away or after a certain action? :)

I also get SourceMap warnings on the DevTool like (on master):

DevTools failed to parse SourceMap: https://cdn.jsdelivr.net/npm/@tensorflow/tf-converter.min.js.map

DevTools failed to parse SourceMap: https://cdn.jsdelivr.net/npm/@tensorflow/tf-core.min.js.map

The error I have shows up on the terminal when running either dev NPM scripts (edit: not anymore).

The `ENV` bit appears invalid and causing the script to fail, and
normally that's not needed when setting environmental variables on a
script execution.
And re-updates the Stylelint ignore list.
Re: #55 (comment)
Since `location.pathname` returns `/dev` regardless of the URL ending in
`/dev.html` or `/dev` (a fun and surprising thing I know), it's better
to check it agains `/dev` (otherwise it will be `false` when someone is
on the dev page).
Copy link
Collaborator

@Berkmann18 Berkmann18 left a comment

Choose a reason for hiding this comment

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

The build works as far as I can see, I looked at the LHCI artifacts from a836a6b (master) and 3e85816 (this PR) and here's the results (script used mentioned below):

/dev changes:

Commit Perf A11y BP SEO PWA Average
a836a6b 82% 98% 79% 100% (33.33%, 33.33%, 85.71%) 73.05%
3e85816 -1% -6% - - (-, -, -) -1%

lh-avg '82/98/79/100/(1, 1, 6)' '81/92/79/100/(1, 1, 6)' -psd -f md -n 'a836a6b,3e85816'

/ changes:

Commit Perf A11y BP SEO PWA Average
a836a6b 78% 100% 79% 100% (33.33%, 33.33%, 85.71%) 72.77%
3e85816 +7% - - - (-, -, -) +1%

lh-avg '78/100/79/100/(1, 1, 6)' '85/100/79/100/(1, 1, 6)' -psd -f md -n 'a836a6b,3e85816'

So it does seem that the bundling is improving the performance, however, the dev page's bundle should be looked at.

Note: LHCI does tests on an Emulated Nexus 5X (so mobile versus desktops which I've tested on all along).

@amirhmk
Copy link
Collaborator Author

amirhmk commented Apr 27, 2020

@Berkmann18 I see what you mean about the minified versions. They were not supposed to be touched so we only need the minified version. Good call!

@Berkmann18
Copy link
Collaborator

Berkmann18 commented Apr 27, 2020

@Berkmann18 I see what you mean about the minified versions. They were not supposed to be touched so we only need the minified version. Good call!

Yeah, otherwise users would have to wait for the full uncompressed resources, which is bad.

Copy link
Collaborator

@Berkmann18 Berkmann18 left a comment

Choose a reason for hiding this comment

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

There are still some areas to improve, especially for /dev but it's better to have this improved version onto production; which would show us even better results, especially for best practices, PWA and quite possibly performance (assuming the relevant resources get a cache policy).

This way, it builds the site based on the correct directory (otherwise
it break the deployment).
Copy link
Owner

@peddybeats peddybeats left a comment

Choose a reason for hiding this comment

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

Thanks Amir!

@Berkmann18 Berkmann18 merged commit 4857c7f into master Apr 27, 2020
@Berkmann18 Berkmann18 deleted the add-bundler branch April 27, 2020 11:04
Berkmann18 added a commit that referenced this pull request Apr 27, 2020
Since there's been a recent drop in the a11y score (6% drop in #55), it
makes sense to improve the relevant contrast
Berkmann18 added a commit that referenced this pull request Apr 27, 2020
Since there's been a recent drop in the a11y score (6% drop in #55), it
makes sense to improve the relevant contrast
Berkmann18 added a commit that referenced this pull request May 2, 2020
Since there's been a recent drop in the a11y score (6% drop in #55), it
makes sense to improve the relevant contrast
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Bundler
3 participants