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

refactor(gatsby-plugin-manifest): misc code and doc updates #12757

Merged
merged 21 commits into from
Apr 8, 2019

Conversation

moonmeister
Copy link
Contributor

Description

Code and Docs updates for gatsby-plugin-manifest

  • Finished moving to module imports
  • Finished migrating off of bluebird and onto async/await (order of magnitude speed increase)
  • Major doc updates
  • Improve logging output for generation

@moonmeister moonmeister requested review from DSchau and a team March 22, 2019 12:30
@moonmeister moonmeister changed the title gatsby-plugin-manifest code and doc updates refactor: (gatsby-plugin-manifest) misc code and doc updates Mar 23, 2019
@moonmeister
Copy link
Contributor Author

One thing I did want to open up for discussion on this is using gatsby-plugin-sharp as a peer dependency instead of directly depending on sharp. I'm not sure if there would be any benefits or drawbacks either way. It crossed my mind and wanted to see what others thought.

Copy link
Contributor

@DSchau DSchau left a comment

Choose a reason for hiding this comment

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

This looks great! I'll take a deeper dive on this in a bit--but could you clarify this?

Finished migrating off of bluebird and onto async/await (order of magnitude speed increase)

Do you have any metrics/benchmarks/etc. for this?

@moonmeister
Copy link
Contributor Author

@DSchau haha I just mentioned this chat but I'll put it here for everyone:

when I eliminated the final line of code requiring bluebird and replaced it with a native promise I saw icon generation drop from between 200ms and 900ms to down to ~13ms.

I'm running latest node 10 lts on linux.

@moonmeister
Copy link
Contributor Author

Rebased after 2 different manifest related PRs and fixed 2 bugs from #12794

@moonmeister
Copy link
Contributor Author

Added tests and refactored the icon options merge as described here: #12794 (comment). Also, fixed bug that was not letting config merge happen (thank you testing).

@moonmeister
Copy link
Contributor Author

Moved bug fixes to #12907

@moonmeister
Copy link
Contributor Author

moonmeister commented Mar 29, 2019

@DSchau Well, bench marking proved I was wrong. 😞 Evidently it was just a strange fluke that I saw execution time drop. That or my bench mark is inaccurate. 🤷‍♂️ I've added a bench mark for testing the execution time of onPostBootstrap in the manifest plugin.

@wardpeet
Copy link
Contributor

I resolved the conflicts but I wasn't sure of

<<<<<<< manifest-cleanup -- Incoming Change
The `icon_options` object may be used to iteratively configure the [default icon array](#automatic-mode-configuration). Any options included in this object will be merged with each item of the `icons` array. This may be used in Automatic and Hybrid modes.
=======
The `icon_options` object may be used to iteratively add configuration items to the `icons` array. Any options included in this object will be merged with each object of the `icons` array (custom or default). Key value pairs already in the `icons` array will take precedence over duplicate items in the `icon_options` array.
>>>>>>> master -- Current Change

I used master when merging but feel free to change back what you had on this PR

@moonmeister
Copy link
Contributor Author

You made the right call.

@moonmeister
Copy link
Contributor Author

@DSchau @wardpeet Anything I can do to get this done or is it ready to be merged? The only other thing that's been on my mind is whether we should remove the dependency on sharp and use gatsby-plugin-sharp. I'm completely unaware of why we' prefer one method or the other.

@wardpeet
Copy link
Contributor

wardpeet commented Apr 8, 2019

Thanks for pinging!

This PR is pretty hard to review as so many things have changed, you added functionality & changed a lot of tests. It looks like you didn't change behaviour in the tests except omitting data but now it's really hard to actually validate if your changes are not breaking.

I'll probably ask some dumb questions in the review to make sure I've understood everything.

@moonmeister
Copy link
Contributor Author

moonmeister commented Apr 8, 2019

@wardpeet Sorry! I'll keep PRs smaller in the future. yes, there are no new features. I just organized docs and tests. In the test work I found a couple bugs I squashed. Also each commit is generally one change. Maybe reviewing each commit individually might be helpful. I'll be around to answer questions

Copy link
Contributor

@wardpeet wardpeet left a comment

Choose a reason for hiding this comment

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

I've tested the code with the old tests and it seems to work fine.

I moved to createContentDigest from gatsby instead of our own.

@wardpeet wardpeet added the bot: merge on green Gatsbot will merge these PRs automatically when all tests passes label Apr 8, 2019
@moonmeister
Copy link
Contributor Author

moonmeister commented Apr 8, 2019

@wardpeet Awesome. I don't think the built-in content digest existed when I wrote that code originally. I think it was still in discussion glad you moved over to that! Thanks!

@wardpeet wardpeet merged commit 01670e1 into master Apr 8, 2019
@wardpeet wardpeet changed the title refactor: (gatsby-plugin-manifest) misc code and doc updates refactor(gatsby-plugin-manifest): misc code and doc updates Apr 8, 2019
@wardpeet wardpeet deleted the manifest-cleanup branch April 8, 2019 09:00
@sidharthachatterjee
Copy link
Contributor

Thanks for all your work in this @moonmeister!

Published in gatsby-plugin-manifest@2.0.27

johno pushed a commit to jlengstorf/gatsby that referenced this pull request Apr 10, 2019
…s#12757)

## Description

Code and Docs updates for `gatsby-plugin-manifest`

- Finished moving to module imports
- Finished migrating off of `bluebird` and onto async/await (order of magnitude speed increase)
- Major doc updates
- Improve logging output for generation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: merge on green Gatsbot will merge these PRs automatically when all tests passes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants