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

'added' event is not emited until child has access to stage #68

Merged
merged 5 commits into from
Aug 1, 2018
Merged

'added' event is not emited until child has access to stage #68

merged 5 commits into from
Aug 1, 2018

Conversation

fireveined
Copy link
Contributor

@fireveined fireveined commented Jul 28, 2018

Description

Now we wait with emiting 'added' event until child has access to stage, to ensure it will be mediated.

Related Issue

#67

Motivation and Context

In our project we build views from bottom to top, so none of them were mediated.

How Has This Been Tested?

Tested in use case from linked issue and in our project.

Types of changes

  • Updated docs / Refactor code / Added a tests case (non-breaking change)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@tiagoschenkel tiagoschenkel self-assigned this Jul 31, 2018
@tiagoschenkel
Copy link
Member

@fireveined, please take a look at the failing travis log.

This message is everywhere:

Module not found: Error: Can't resolve 'pixi.js' in '/home/travis/build/RobotlegsJS/RobotlegsJS-Pixi/src/robotlegs/bender/extensions/contextView/pixiPatch'

Unfortunately the peerDependency doesn't work as expected since yarn is being used to manage the dependencies. And, as far as I'm concerned, even the npm package manager is not solving properly the peer dependencies. When a peer dependency is added, the binaries are not downloaded when running yarn install or npm install so this is why the job is failing. Remember that the travis integration build runs on a fresh environmnt so everything is wiped out before. Locally it could work but is because the node_modules folder was not cleaned.

Would be possible to revert your last commit?

Regarding the version of the pixi.js library, I think that makes sense to accept a more broad version range, like ^4.0.0. I prefer to use this notation instead the 4.x notation, since both ways are semantically correct and will produce the same result (you can check this here).

But in any case, I would prefer to change the version range of the dependencies in another issue/pull request. Currently I don't know exactly how the this plugin is used, nor how often one development team is updating the pixi.js dependency on each project. Would be good to discuss this in a separate topic.

@tiagoschenkel
Copy link
Member

Would be good to update also the CHANGELOG.md file located in the root of this project.

Please add one line describing briefly the changes made in the changelog, then a reference to this pull request and to the issue being closed.

You can add an entry line here.

@tiagoschenkel
Copy link
Member

Problem related to peerDependencies solved. Accepting other changes.

@tiagoschenkel
Copy link
Member

#72 extends the concept to removed event.

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

Successfully merging this pull request may close these issues.

2 participants