-
Notifications
You must be signed in to change notification settings - Fork 67
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
adapted to work with Polymer 3.0 #99
Conversation
Don't know if my change breaks something on .html library code 🤔, the ES module version works fine and I'm already using it in my projects. I would suggest you to make a new release ES module only, following the recent updates on Bower and Polymer, so if people can't migrate to v3.0 they just lock version number. 🎉🎉🎉🎉 |
News here? 🙃 |
polymer-redux@next branch created, awaiting a review for the imports. |
…n package.json (vaadin-combo-box needs work)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some changes according to Import dependencies, https://www.polymer-project.org/blog/2017-08-23-hands-on-30-preview.html
src/index.js
Outdated
@@ -1,8 +1,9 @@ | |||
import window from 'global/window'; | |||
import console from 'global/console'; | |||
import {get} from '@polymer/polymer/lib/utils/path'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import {get} from '../../@polymer/polymer/lib/utils/path';
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right! I think it will be a bit longer to fix all the repo (I have first to understand it 😄 ) so it will take some commits to have everything working fine... Vaadin combo box hasnt yet a v3 version so I will work on that first maybe
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because polymer-redux is a library and not part of the application need to make sure the imports following other polymer libraries.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unfortunately it seems that the "suggested" way of import from the Polymer team does not always work, I cant get the tests to run and the code to be built at the sime time, relatives paths are always a mess.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With Webpack builds this problem is solved, so I'm still using my fork on my company projects. Can't get how to make things mergeable with the actual repo structure 😞 (HTML Imports stuff)
@@ -1,3 +1,5 @@ | |||
import { Element as PolymerElement } from '../../@polymer/polymer/polymer-element'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is wrong. lib/index.js
is generated from npm run build
.
Also why are you including this here when we don't use PolymerElement anywhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should maybe document this on the README actually of how to build the repo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
didn't know that... ok maybe it's better if I leave the repo adaptation to v3 to you, the main thing that is required to make polymer-redux working in a v3 project was that
import {get} from '../../@polymer/polymer/lib/utils/path';
thing, so in the meanwhile I can work with my fork.
Sorry for the inconvenience
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, don't be sorry. I'm snowed under at work, this is great I really appreciated it 👍
Just run npm run build
then commit the changes and you're done 🎉
Added changes from tur-nr#99
@tur-nr @moebiusmania We have a working version here: https://github.com/NERDDISCO/polymer-redux Is this PR still active or should we open a new one? demo: https://github.com/NERDDISCO/VisionLord/blob/feat/redux/src/reduxStore.js |
See polymer-3 branch. |
fixes #97