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

effector/babel-plugin plugin doesn't process factories field #732

Closed
rushelex opened this issue Jun 23, 2022 · 11 comments
Closed

effector/babel-plugin plugin doesn't process factories field #732

rushelex opened this issue Jun 23, 2022 · 11 comments
Labels
question Further information is requested

Comments

@rushelex
Copy link

What is the current behavior:
If you add the factories field in the effector/babel-plugin configuration and specify the actual path of the factory that is imported into the model files, then the sid for units is specified without additionally specifying the sid of the factory.

Please provide the steps to reproduce and if possible a minimal demo of the problem via https://share.effector.dev, https://codesandbox.io or similar:
https://stackblitz.com/edit/effector-react-1eg5yl?file=src%2Fmodel.ts

What is the expected behavior:
The factories field is handled correctly and the sid of the units created by the factory is different.

Which versions of effector packages, and which browser and OS are affected by this issue? Did this work in previous versions of effector?:
OS: any
Browser: any

Did this work in previous versions of effector?

I don't know.

@rushelex rushelex added bug Something isn't working needs triage labels Jun 23, 2022
@Drevoed
Copy link
Contributor

Drevoed commented Jul 17, 2022

Factories field must point to the factory path relative to root folder.

So in your case it is fixed by setting factories like this:

{
  factories: ['./src/shared/lib/effector/store']
}

@rushelex
Copy link
Author

So in your case it is fixed by setting factories like this:

No, it doesn't work. In addition, they wrote in the telegram chat that the path should be exactly the same as that written in the import construct.

@sergeysova
Copy link
Collaborator

sergeysova commented Jul 18, 2022

Write full path from the root into the factories:

https://stackblitz.com/edit/effector-react-otbbgz?file=.babelrc

-"factories": ['./shared/lib/effector/store'],
+"factories": ["./src/shared/lib/effector/store"]

Also, I removed babel option from react() plugin in vite.config.js. Prefer to use babel() with extensions list

Result with different sid's:
image

@rushelex
Copy link
Author

But that doesn't solve the problem...

Look at this code. There are comments "It works" and "It doesn't work". That is the meaning of this issue. The factory code doesn't work properly even if there are different sids.

@rushelex
Copy link
Author

ezgif-5-f597a5718c.mp4

@sergeysova
Copy link
Collaborator

sergeysova commented Jul 18, 2022

Oh, there wasn't a comment about the expected behavior in the code/issue. Only about sid

@sergeysova
Copy link
Collaborator

sergeysova commented Jul 18, 2022

Because the sids is different, you need to search for problem in your fabric implementation, not in the babel-plugin.

@sergeysova
Copy link
Collaborator

sergeysova commented Jul 18, 2022

You made CATASTROPHIC hard code and made a bug inside it:

const getIntersectingEvents = <
  TargetStore extends Store<any>,
  EventMap extends Array<[TargetStore, Event<void>]>
>(
  targetStore: TargetStore,
  eventMap: EventMap
): Event<void>[] => {
  return eventMap
-    .filter(([store]) => store.shortName !== targetStore.shortName)
+    .filter(([store]) => store.sid !== targetStore.sid)
    .map(toEvent);
};

Proof: https://stackblitz.com/edit/effector-react-otbbgz?file=src%2Fmodel.ts

@sergeysova
Copy link
Collaborator

sergeysova commented Jul 18, 2022

I totally recommend rewriting this code without using .sid or .shortName.

@sergeysova sergeysova added question Further information is requested and removed bug Something isn't working labels Jul 18, 2022
@sergeysova
Copy link
Collaborator

sergeysova commented Jul 18, 2022

The previous code works well because it really had different short names. But stores made inside fabric will have the same name because inside the fabric function variable has the same name for each fabric instance, there are only changes to the name after fabric call.

const $a = createStore(0); // .shortName = "$a" .sid = "1d5fxc2"
const $b = createStore(1); // .shortName = "$b" .sid = "8skdx1s"
function factory() {
  const $demo = createStore(0) // .shortName = "$demo" .sid = "2i0cm"
  return $demo;
}

const $a = factory(); // .shortName = "$demo" .sid = "9ddkba|2i0cm"
const $b = factory(); // .shortName = "$demo" .sid = "sc8abs|2i0cm"

That's why you don't need to write code that depends on these internal properties. It is a really fragile way to write code

@rushelex
Copy link
Author

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants