Skip to content

removes exjs dependency #42

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

Merged
merged 16 commits into from
Dec 29, 2018

Conversation

mejackreed
Copy link
Contributor

@mejackreed mejackreed commented Dec 22, 2018

Part of #41

Though, I'm not sure how much this does in reducing actual bundle size.

@edsilv
Copy link
Member

edsilv commented Dec 27, 2018

Hey, please see the failing getRangeByPath test I added.
I think it's related to this: https://github.com/IIIF-Commons/manifesto/pull/42/files#diff-01f13f6d1a0f04758601d8128e56b2efL1429

const subRanges: IRange[] = topRange.getRanges();
this._allRanges = this._allRanges.concat(subRanges.en().traverseUnique(range => range.getRanges()).toArray());
const subRanges: IRange[] = topRange.getRanges();
this._allRanges = this._allRanges.concat(subRanges);
Copy link
Member

Choose a reason for hiding this comment

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

Could reduce it down using something like:

const reducer = (acc, next) => {
  acc.push(next);
  const nextRanges = next.getRanges();
  if (nextRanges.length) {
     return nextRanges.reduce(reducer, acc);
  }
  return acc;
}

and use it like:

const flatRanges = topRange.getRanges.reduce(reducer, [topRange]);

If it needs to be unique, could make it a Set or a Map mapped by the id field. (if this is the failing test that you mentioned)

Copy link
Member

Choose a reason for hiding this comment

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

Yep, we need a flat array of unique ranges. Using Set sounds good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the comments here. I've used your approach @stephenwf and pushed that up in
4576dc8.

@@ -27,7 +27,8 @@ namespace Manifesto {

public setLabel(value: string): void {
if (this.label && this.label.length) {
var t: Manifesto.Language = this.label.en().where(x => x.locale === this.defaultLocale || x.locale === Manifesto.Utils.getInexactLocale(this.defaultLocale)).first();
console.log(this.label);
Copy link
Member

Choose a reason for hiding this comment

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

A couple of console logs made its way in 👀

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for catching that.

@mejackreed
Copy link
Contributor Author

Thanks for the great review (and the additional tests added). Is there a way you would like me to resolve the dist conflicts?

Also I noticed that the uniqueness needed in the added test wasn't caught when just using an Array. @edsilv is there a manifest/scenario where I could write a failing test for that?

@edsilv
Copy link
Member

edsilv commented Dec 28, 2018

I've altered the biocrats.js test to check the number of returned ranges. The root range was being included when it didn't have an id. Root ranges without ids are "placeholders" for generating trees I believe.

I've also removed the dist directory, but this doesn't seem to have fixed the conflicts?

@mejackreed mejackreed force-pushed the remove-exjs-dependency branch from 38ec1e9 to 4a0bf85 Compare December 28, 2018 21:01
@mejackreed
Copy link
Contributor Author

I've force pushed a rebase that should handle the dist thrashing. I'm going to submit a separate PR that should help with the release process for this package, so that built versions do not need to be submitted in PR's or committed.

@edsilv
Copy link
Member

edsilv commented Dec 29, 2018

Cool. Well, the UV seems happy now. Think it's mergeable?

@edsilv edsilv merged commit 3217f1e into IIIF-Commons:master Dec 29, 2018
@mejackreed mejackreed deleted the remove-exjs-dependency branch December 30, 2018 16:41
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.

3 participants