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

fix(test): Update to support latest version of markedjs #1434

Closed

Conversation

lasjorg
Copy link
Contributor

@lasjorg lasjorg commented Dec 1, 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

Markdown projects are failing tests they shouldn't with the latest version 4 of markedjs when loaded using Codepen settings or a script element.

TL;DR explanation. We had a typeof check that was failing. typeof marked can return 'function' or 'object' depending on the version of the library and how it is loaded.


Forum: https://forum.freecodecamp.org/t/markdown-previewer-solution-fails-tests-5-and-6-despite-functioning-as-intended/484737

Example Codepen with the old and new versions of tests and library.
https://codepen.io/lasjorg/pen/jOGEqbL?editors=1010

Note that marked.parse(props.markdown, { renderer: renderer }) (Line: 108) is used in this example.


Very much not TL;DR explanation: I apologize in advance for the long and somewhat confusing explanation. Frankly, at this point, I'm a little confused myself.

When version 4 of markedjs is loaded using the JS section in the Codepen settings or as a script element typeof marked returns 'object'. Version 3 and below of markedjs returns 'function'.

When using the import syntax on Codepen.

// is logged out as an object when logged in the JS code box
`import * as marked from "https://cdn.skypack.dev/marked@4.0.5";`

// is logged out as a function when logged in the JS code box
`import { marked } from "https://cdn.skypack.dev/marked@4.0.5";`

However, if you do the same console.log in the browser console manually before running the tests it will return undefined. It being undefined will (afaict) cause us to load our version of the library found in src\index.js when you run the test. Which is (or was) an old version that would also report typeof 'function'. As far as I can tell, it looks like when the camper uses the Codepen import syntax we will always load the script found in src\index.js.

The test internally uses the markedjs library. As long as typeof marked doesn't return undefined we (afaict) use the version that is loaded by the project being tested. If the project does not load the library, i.e. typeof marked does return undefined we use our own version. That code is in src\index.js inside the function loadMarked.

This is why currently when using import on Codepen it will make projects that are using version 4 of markedjs pass the tests again as it causes the old version of the library to be loaded which will not fail the typeof marked === 'function' check.

If I'm right about this then another option instead of this PR would be to never use the version loaded by the project and always use our own version for the tests. This approach is likely more future-proof. It would also be preferable to use an npm package for maintainability instead of a CDN link. The only downside I can see is it would increase the bundle size.


Side note: We should probably update the example project to use the latest version of the library as well. It would be a small PR. I might do it later at some point.

@lasjorg lasjorg requested a review from a team as a code owner December 1, 2021 22:08
@lasjorg
Copy link
Contributor Author

lasjorg commented Dec 1, 2021

Just to make sure someone sees this @scissorsneedfoodtoo @ShaunSHamilton

@ShaunSHamilton
Copy link
Member

@lasjorg Thanks, for the ping.

Just to clarify, I see both versions pass all tests using both ways to import import * as marked and import { marked }. I am not sure if I am missing something, because I do see the difference ('object' vs 'function')

@lasjorg
Copy link
Contributor Author

lasjorg commented Dec 1, 2021

both versions pass all tests

When you say both versions, are you talking about the old and new test scripts?

If so I tried to explain it in the longer explanation. When using import on Codepen we fall back to using the version of markedjs that we load in src/index.js (for the internal usage of markedjs). And that version of markedjs is an old version 0.5.0 which won't fail the typeof check.

So when that old version of markedjs is loaded the old test will still work even if the project is using version 4 of markedjs, as we will not be using that version (the one the project is using) for the internal tests but the old version loaded in src/index.js.

As I said, it's confusing and hard to explain.

@ShaunSHamilton
Copy link
Member

When you say both versions, are you talking about the old and new test scripts?

No, v3 and v4 of Marked.js

@lasjorg
Copy link
Contributor Author

lasjorg commented Dec 1, 2021

To be clear, if you want to see the old test fail use the script element that loads markedjs version 4 in the HTML.

When you use import we do not use that version internally for the tests so it doesn't fail the typeof check that it will when loading markedjs version 4 in the HTML. When loaded in the HTML or settings we do use that version internally.

@lasjorg
Copy link
Contributor Author

lasjorg commented Dec 1, 2021

It's really hard to explain. So sorry about that.

  1. If you load markedjs V4 in the HTML or the Codepen Setting the old test will fail.

  2. If you load markedjs V4 as an import the old test will pass.

  3. The reason the old test passes when using import is that we won't internally be using that version for the testing but the older version we load in src/index.js and that version is "compatible" with the old test typeof check.

When using import the library will show up as undefined and trigger the code in src/index.js to add the version found there. I don't know why it is undefined, but it is. You can check it in the browser console. But you have to do it before running the test, otherwise running the test will load the library from src/index.js.

@ShaunSHamilton
Copy link
Member

That does clear that confusion up. Thank you, for bearing with me.

Then, in this case:

When using import the library will show up as undefined and trigger the code in src/index.js to add the version found there

Then I agree with this:

If I'm right about this then another option instead of this PR would be to never use the version loaded by the project and always use our own version for the tests.


It would also be preferable to use an npm package for maintainability instead of a CDN link.

I do not understand why it would be preferable - both allow versioning? Also, it appears the CDN is ~1/3 the size of the npm package.

@lasjorg
Copy link
Contributor Author

lasjorg commented Dec 1, 2021

I do not understand why it would be preferable - both allow versioning? Also, it appears the CDN is ~1/3 the size of the npm package.

Managing dependencies using package.json is far more maintainable and seeing as the version in src/index.js is as old as it is, it clearly has never been updated. So no one has paid any attention to it.

Using a CDN link buried inside some code no one ever looks at is hardly a very maintainable approach to internal dependencies.

@lasjorg lasjorg mentioned this pull request Dec 2, 2021
10 tasks
@lasjorg
Copy link
Contributor Author

lasjorg commented Dec 2, 2021

Likely closing this in favor of #1435 but I will keep this open for now.

@scissorsneedfoodtoo
Copy link
Contributor

scissorsneedfoodtoo commented Dec 13, 2021

@lasjorg, thanks for your patience, and for the very detailed breakdown of the problem, possible solutions, and the pros and cons of each solution.

After reading through the discussion you had with @ShaunSHamilton, and trying this and your other PR, I believe adding Marked as an npm dependency is better in this case. Like you mentioned, it's more maintainable and easier to understand, even if it does increase the bundle size a bit.

Will close this in favor of #1435.

@lasjorg lasjorg deleted the fix/support-new-version-of-markedjs branch December 13, 2021 23:12
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