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

Clone document on addStatic. #14

Merged
merged 11 commits into from
Jun 6, 2024
Merged

Clone document on addStatic. #14

merged 11 commits into from
Jun 6, 2024

Conversation

aljones15
Copy link
Contributor

This seems like the easiest solution to the document mutation issue.
If we don't want a dep we could use JSON.parse(JSON.stringify(document)) as the jsonld doc should be just JSON.

@aljones15 aljones15 self-assigned this May 29, 2024
@aljones15 aljones15 requested review from davidlehn and dlongley May 29, 2024 19:53
@codecov-commenter
Copy link

codecov-commenter commented May 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.40%. Comparing base (9ef1abd) to head (b0bc444).

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #14      +/-   ##
==========================================
+ Coverage   98.27%   98.40%   +0.12%     
==========================================
  Files           2        2              
  Lines         116      125       +9     
==========================================
+ Hits          114      123       +9     
  Misses          2        2              
Files Coverage Δ
lib/JsonLdDocumentLoader.js 98.33% <100.00%> (+0.13%) ⬆️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9ef1abd...b0bc444. Read the comment docs.

@aljones15
Copy link
Contributor Author

@dlongley structuredClone works, but not in node <= 16 came in to node in 17 according to the page you linked to.

@aljones15 aljones15 requested a review from dlongley May 29, 2024 20:45
Copy link
Member

@dlongley dlongley left a comment

Choose a reason for hiding this comment

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

Thanks!

@dlongley
Copy link
Member

We should drop the testing for node 16 and bump the suggestion in the package.json (if any) to node 18+.

@aljones15
Copy link
Contributor Author

@dlongley while bumping engines we had previously agreed is not major is adding a new method that is node >= 17 only and only supported in somewhat recent browsers breaking and hence a major change?

@dlongley
Copy link
Member

Given our package.json "node recommendation" policy, it's not a breaking change. We've just gotten behind on upgrading this particular package's expected node value here. We only support LTS+. So we could do the node >= 18 bump in a separate PR and release first, but it would still be a non-breaking release. Since there's no real difference there other than churn, let's roll this all together.

The supported browsers and node versions are "current" and LTS+, and think we're good there for the structuredClone feature:

https://caniuse.com/?search=structuredClone

If people need to polyfill this in old browsers I imagine they're already doing so in their build pipelines or should be. As a matter of policy, our individual libraries do not provide support for legacy platforms.

@davidlehn
Copy link
Member

  • Just to check, the speed and memory hit on this is ok? I imagine in most use cases no one one notice. But there's also no choice for those rare use cases with huge resources. "Use another loader" is a choice of course.
  • "engine" spec issues aside, the definite newer API usage here could break user code running in 16.x (for whatever legacy reasons that is being used). Are you all sure you want to do that?

@davidlehn
Copy link
Member

  • "engine" spec issues aside, the definite newer API usage here could break user code running in 16.x (for whatever legacy reasons that is being used). Are you all sure you want to do that?

At a minimum, I think the changelog should note support or a polyfill for structuredClone is required and mention whatever version of Node.js that is.

aljones15 and others added 3 commits May 31, 2024 15:24
- Update supported Node.js version in README.
- Add notes about document being cloned.
- Add docs for `addStatic`.
- Update changelog with more details about cloning and potential need
  for a polyfill.
Copy link
Member

@davidlehn davidlehn left a comment

Choose a reason for hiding this comment

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

I'm not convinced this is needed, but we'll try it out, and adjust the API if needed.

@davidlehn davidlehn merged commit 7949d4c into main Jun 6, 2024
5 checks passed
@davidlehn davidlehn deleted the add-static-clones-documents branch June 6, 2024 01:29
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.

4 participants