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.

Sign up for GitHub

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

Jump to bottom

feat(minify-json-ld): minifies jsonLd output #487

Merged
merged 3 commits into from
Sep 25, 2020
Merged

feat(minify-json-ld): minifies jsonLd output #487

merged 3 commits into from
Sep 25, 2020

Conversation

igormartimiano
Copy link
Contributor

Description of Change(s):

Minifies the jsonLd content before rendering it on <Head>
I'm not entirely sure if this is the most efficient way of doing this, but definitely reliable and doesn't add any extra deps. I'd rather have the extra computational work than no compression at all.

Closes #263 (comment)

Some things to help get a PR reviewed and merged faster:

  • Have you updated the documentation to go with your changes?
    Not sure if it should be mentioned anywhere in the docs, up to suggestions. Would be no problem to do it
  • Have you written or updated unit tests?
    Didn't find any jsonLd related unit tests, and I didn't write any
  • Have you written an integration test in the test app supplied?
    Didn't write it but I tested on the test app and also ran cypress locally

@garmeeh
Copy link
Owner

garmeeh commented Sep 24, 2020

@igormartimiano very nice solution 👏

🤔 I just did a quick search and on larger data sets it may cause an issue but for this library I can't imagine that being an issue.

I wonder should we add a flag that to opt out? Just thinking if any site has an issue with it they can quickly opt out.

Although this may be overkill...

@igormartimiano
Copy link
Contributor Author

@garmeeh Thanks, glad to help!
Could you provide resources about this particular issue? Wasn't aware of that. If it's critical enough, I think we should try to minify doing something else instead of adding a flag to opt out of the feature. I see that kind of flag as a workaround to a bug that shouldn't be there in the first place and wouldn't go that route unless it is absolutely necessary to provide the feature with a certain implementation.
When I have enough time I'll also give it a search. I really would have to get a better grasp of the issue to discuss.
But, ultimately, from my experience schemas aren't very large data sets anyway.

@garmeeh
Copy link
Owner

garmeeh commented Sep 25, 2020

@igormartimiano yeah I totally agree, I think I might be just a little too cautious. As I said, I only gave it a quick search so really shouldn't base any assumptions on a that 😅

I think as it stands it is a nice addition and does not add any overhead. If an issue arises it can be addressed then.

I'm happy to merge and release this if you are?

@igormartimiano
Copy link
Contributor Author

@garmeeh Alright then, looks good to me 👌🏻

@garmeeh garmeeh merged commit 57279df into garmeeh:master Sep 25, 2020
@garmeeh garmeeh mentioned this pull request Jun 21, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@garmeeh garmeeh garmeeh approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Minify JSON-LD output
2 participants
@igormartimiano @garmeeh