Skip to content

create single dist esm file to clear circular dependencies #2022

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

Conversation

iambumblehead
Copy link

@iambumblehead iambumblehead commented Sep 24, 2024

Feel free to ignore this PR or to suggest any changes at all.

resolves #2019

@iambumblehead iambumblehead force-pushed the generate-dist-esm-file-to-clear-circular-dependencies branch from 89d337a to 91bbb7c Compare September 24, 2024 22:40
@iambumblehead
Copy link
Author

If 5.x is merged sometime soonish, I may have energy to attempt a PR that removes circular dependencies.

@bdurrer
Copy link
Contributor

bdurrer commented Sep 25, 2024

FYI, I once tried to figure out the needed changes in another issue. One central point was that the toOrderedSet and toOrderedMap are the only reason for a loop between the large ordered and unordered collection types.
While the removal would reduce the beauty of the API, the migration from myMap.toOrderedMap() to OrderedMap(myMap) would be relative easy for the users,

@jdeniau jdeniau deleted the branch immutable-js:5.x October 17, 2024 22:04
@jdeniau jdeniau closed this Oct 17, 2024
@jdeniau
Copy link
Member

jdeniau commented Oct 17, 2024

Sorry I did merge 5.x branch, it did close this.
Could you reopen it based on the main branch ?

@iambumblehead
Copy link
Author

wonderful thanks! I'm in the middle of things atm and am planning to get away from the computer for a solid couple of days or a week or so so I don't know when I will circle back :)

@iambumblehead
Copy link
Author

The variety and depth of the circular definitions here is incredible. Separating Seq from Collection, CollectionImpl and Operations requires one to make many changes and decisions on different things.

@iambumblehead
Copy link
Author

iambumblehead commented Nov 7, 2024

the changes are rebased to the PR here #2022

I spent a few hours breaking the Seq->Collection circular dependency but am stopping. Changes in one place force changes in the next place and so on and I'm not ready to spend so much more time on it.

Here are two areas I believe could be handled as separate PRs, so that subsequent removal of circular dependencies will be a little easier.


First area, use an imported function to define the collection prototype.

The collection prototype currently defines this way at CollectionImpl.js, here,

const CollectionPrototype = Collection.prototype;
CollectionPrototype[IS_COLLECTION_SYMBOL] = true;
CollectionPrototype[ITERATOR_SYMBOL] = CollectionPrototype.values;
CollectionPrototype.toJSON = CollectionPrototype.toArray;
CollectionPrototype.__toStringMapper = quoteString;
CollectionPrototype.inspect = CollectionPrototype.toSource = function () {
  return this.toString();
};
CollectionPrototype.chain = CollectionPrototype.flatMap;
CollectionPrototype.contains = CollectionPrototype.includes;

It currently forces Seq.js to depend on CollectionImpl.js for these prototype definitions. If they are assigned through a function that is imported separately, Seq.js can later import and use that function instead of CollectionImpl.js.

import CollectionProtoAssign from './CollectionProtoAssign';

const CollectionPrototype = CollectionProtoAssign(Collection.prototype);

Second area, update CollectionImpl.js methods to call imported functions that do not directly reference this.

+const sharedFindEntry(seq, predicate, context, notSetValue) {
+  let found = notSetValue;
+  seq.__iterate((v, k, c) => {
+    if (predicate.call(context, v, k, c)) {
+     found = [k, v];
+     return false;
+   }
+  });
+  return found;
+}

  findEntry(predicate, context, notSetValue) {
-   let found = notSetValue;
-   this.__iterate((v, k, c) => {
-     if (predicate.call(context, v, k, c)) {
-       found = [k, v];
-       return false;
-     }
-   });
-   return found;
+   return sharedFindEntry(this, predicate, context, notSetValue);
  },

Later on, when Seq is separated from Collection, it can use these functions and will not need to extend Collection or its inheritors and calls like these can be used,

sharedFind(collection, predicate, context, notSetValue);
sharedFind(seq, predicate, context, notSetValue);

Factory functions at Operations.js may need 'this' removed as well, to also be used by both sequences and collections.

@jdeniau
Copy link
Member

jdeniau commented Nov 7, 2024

Thank you for your energy @iambumblehead I will review your PR and merge this asap

@jdeniau
Copy link
Member

jdeniau commented Nov 8, 2024

This has been released in 5.0.2

@iambumblehead
Copy link
Author

want to add; if circular dependencies were removed, it would reduce the amount of code required to be included for smaller dependent packages that use immutable only for limited map and/or list data.

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