Skip to content
This repository has been archived by the owner on May 30, 2023. It is now read-only.

Conversation

lasjorg
Copy link
Contributor

@lasjorg lasjorg commented Dec 2, 2021

Pre-Submission Checklist

  • Your pull request targets the master branch of freeCodeCamp/testable-projects-fcc
  • Branch starts with either fix/, feature/, or translate/ (e.g. fix/signin-issue)
  • You have only one commit (if not, squash them into one commit).
  • Your changes have been tested either locally or using a newly created CDN based on your fork's testable-projects-fcc/build/bundle.js file

Type of Change

  • Small bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds new functionality)
  • Breaking change (fix or feature that would change existing functionality)
  • Add new translation (feature adding new translations)

Checklist:

  • Tested changes locally.
  • Closes currently open issue: Closes #XXX

Description

@ShaunSHamilton here is the new PR with markedjs added as a dependency and the loading code from src/index.js removed.

For anyone else interested, please read #1434 for more information about why this PR is needed.

Tested with example project using markedjs V3 and V4 and tested on Codepen with all the versions you see in the Codepen link below.

Note: bundle.js size is up from 988 KB (1.012.041 bytes) to 1,00 MB (1.059.258 bytes) with the new markedjs dependency.


Codepen with new test:
https://codepen.io/lasjorg/pen/MWEYMXN?editors=1010

@lasjorg
Copy link
Contributor Author

lasjorg commented Dec 3, 2021

@ShaunSHamilton So are we going with this fix?

It would be nice to get this fixed as soon as possible as we are getting forum posts about it.


As an aside, if we want to get the bundle size down looking into removing jQuery might be an option. Or if nothing else, maybe just use the slim version and replace $.getJSON in bar-chart-tests.js with fetch. The slim version excludes the ajax and effects modules.

I believe it can be imported like this.

import jQuery from 'jquery/dist/jquery.slim'

The rest of the usage we have should still work, but I haven't tested it yet. I would also assume that most of the jQuery code is simple enough to replace with plain JS.

@ShaunSHamilton
Copy link
Member

@lasjorg I have just pinged @scissorsneedfoodtoo to also get his input.

From what I can see, this should be fine.

if we want to get the bundle size down looking into removing jQuery might be an option.

If you are unaware, this is one of my favourite part-times - removing jQuery 😁 . So, I am in complete agreement.

@lasjorg
Copy link
Contributor Author

lasjorg commented Dec 12, 2021

@ShaunSHamilton @scissorsneedfoodtoo It would be nice to get this fixed when possible so I'm just pinging you to make sure you didn't forget about it.

@scissorsneedfoodtoo
Copy link
Contributor

@lasjorg, thanks again for your patience with this, and for all the details you provided in this and your other PR.

Like you and Shaun mentioned, there are probably better ways to reduce the bundle size, so I'm not worried about increasing it slightly to improve DX.

Worst case scenario, if we do need to lighten up the bundle size a bit more, we have a detailed record of how to do so in #1434.

Tested locally with a few projects on the forum and everything LGTM. Thanks again for all of your hard work on this, and for helping out the campers who were affected by this 👍 👍

@scissorsneedfoodtoo scissorsneedfoodtoo merged commit 67a09d9 into freeCodeCamp:main Dec 13, 2021
@lasjorg lasjorg deleted the fix/add-markedjs-as-dependency-for-internal-usage branch December 13, 2021 23:13
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants