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

feat(html-resource-plugin, dynamic-element): Add shadow dom support #368

Merged

Conversation

Projects
None yet
4 participants
@michaelw85
Copy link
Contributor

michaelw85 commented Mar 6, 2019

Introduce attribute use-shadow-dom for html only components.

Closes #367

bigopon and others added some commits Jan 19, 2019

feat(html-resource-plugin, dynamic-element): Add shadow dom support
Introduce attribute use-shadow-dom for html only components.

Closes #367
Merge branch 'master' of https://github.com/bigopon/templating-resources
 into 367_add_shadow_dom_attribute

Resolved conflict by re-generating package-lock
@EisenbergEffect

This comment has been minimized.

Copy link
Member

EisenbergEffect commented Mar 8, 2019

@bigopon Can you review this? @fkleuver I think we'd want to add this to vNext template compiler as well, so that it extracts this setting from HTML if present on a template element.

@fkleuver

This comment has been minimized.

Copy link
Member

fkleuver commented Mar 10, 2019

@EisenbergEffect Could you create an issue / TODO for that? There was something else a few days ago as well. I can't really keep track of things like this otherwise 😛

@bigopon

This comment has been minimized.

Copy link
Member

bigopon commented Mar 10, 2019

The change related to new feature lgtm. There are no tests, though we can still merge and I'll write it, or ask @michaelw85 to help a bit more with them :) @EisenbergEffect @fkleuver

@EisenbergEffect

This comment has been minimized.

Copy link
Member

EisenbergEffect commented Mar 10, 2019

Should we enable specifying open or closed mode?

use-shadow-dom="open" and use-shadow-dom="closed"

We can also make use-shadow-dom a shortcut for use-shadow-dom="open"

@bigopon

This comment has been minimized.

Copy link
Member

bigopon commented Mar 10, 2019

@EisenbergEffect right, I forgot that 😄 , and agree

@fkleuver

This comment has been minimized.

Copy link
Member

fkleuver commented Mar 11, 2019

I'm a little concerned about the (dated) CI changes that made their way into this PR. For one, it's yet more deviation, and also some aurelia dependencies were updated. Are we sure this is going to make for a stable release?

@bigopon

This comment has been minimized.

Copy link
Member

bigopon commented Mar 11, 2019

@fkleuver for CI, those config should be removed and cut-release is the only thing needed?

@fkleuver

This comment has been minimized.

Copy link
Member

fkleuver commented Mar 11, 2019

@bigopon You can see here what assumptions it makes. There needs to be a build, test and release script. The defaults are npm run build, npm run test, npm run cut-release. They can easily be overridden as long as there are 3 such scripts. It also assumed package-lock to be up to date (as it uses npm ci) and for there for be some jspm config.

@michaelw85

This comment has been minimized.

Copy link
Contributor Author

michaelw85 commented Mar 11, 2019

The change related to new feature lgtm. There are no tests, though we can still merge and I'll write it, or ask @michaelw85 to help a bit more with them :) @EisenbergEffect @fkleuver

@bigopon I wanted to write a test and noticed none existed yet. I figured since I primarily add existing (proven?) functionality, the effort to figure out how to and get this tested vs risk, was not worth it. If you can easily add a test that would be great although I think I should do it since I created the PR.

Should we enable specifying open or closed mode?

use-shadow-dom="open" and use-shadow-dom="closed"

We can also make use-shadow-dom a shortcut for use-shadow-dom="open"

@EisenbergEffect I am unaware of such options, is there any documentation explaining what these options do and how to apply them?

I'm a little concerned about the (dated) CI changes that made their way into this PR. For one, it's yet more deviation, and also some aurelia dependencies were updated. Are we sure this is going to make for a stable release?

@fkleuver I wanted to provide a PR with a passing CI build but I was wrong to merge another PR into mine. Do you agree if I revert the merge and this discussion continues in the original PR #363

@bigopon

This comment has been minimized.

Copy link
Member

bigopon commented Mar 11, 2019

@EisenbergEffect I am unaware of such options, is there any documentation explaining what these options do and how to apply them?

you can do the following:

<template use-shadow-dom="closed">

translated to:

@useShadowDOM({ mode: 'closed' })
class MyEl {
  ...
}

@bigopon I wanted to write a test and noticed none existed yet. I figured since I primarily add existing (proven?) functionality, the effort to figure out how to and get this tested vs risk, was not worth it. If you can easily add a test that would be great although I think I should do it since I created the PR.

I can add few basic tests related to this, and then if you are still keen, we can collaborate on this afterwards.

@michaelw85

This comment has been minimized.

Copy link
Contributor Author

michaelw85 commented Mar 11, 2019

@bigopon Thanks I will update the PR with the open/closed options.
I was thinking to adding the following scenarios:

  1. No explicit setting use-shadow-dom will use the decoration without options this will make the decorator responsible for setting the default. If for any reason the default would change in the future the attribute and decorator will act identical.

  2. explicit (open/closed) passing the settings object to the decorator.

@EisenbergEffect

This comment has been minimized.

Copy link
Member

EisenbergEffect commented Mar 13, 2019

Sounds great!

@fkleuver

This comment has been minimized.

Copy link
Member

fkleuver commented Mar 13, 2019

@michaelw85 No, I think it was a smart move to merge it so that you could get CI to pass. I'm just pointing out that, when your PR itself is done, we may either want to split off the CI/workflow stuff into a different PR and merge that first so that the scope stays manageable. There are a few things in that PR which I'm not very comfortable with, mainly the major version bump on aurelia-bootstrapper. We have gotten quite some trouble from major version bumps before.

@michaelw85

This comment has been minimized.

Copy link
Contributor Author

michaelw85 commented Mar 13, 2019

Ok, if this PR will not merge any CI changes I still think #363 should be finished and your comments should be resolved in that PR. This will keep things clean an separated.

@bigopon

This comment has been minimized.

Copy link
Member

bigopon commented Mar 17, 2019

Re: major version bump of aurelia-bootstrapper: it's completely safe as it's dev dependencies in jspm packages. It won't affect end users of this package in anyway. About the changes related to new feature, it lgtm. I think it's safe to go.
@fkleuver @EisenbergEffect

michaelw85 added some commits Mar 17, 2019

feat(html-resource-plugin, dynamic-element): Add shadow dom support
Add support for explicit mode declaration
Refactored passing settings to dynamic element to object destructuring
to make it easier to omit any optional setting.

Closes #368
@michaelw85

This comment has been minimized.

Copy link
Contributor Author

michaelw85 commented Mar 17, 2019

Updated code to allow explicitly passing shadow mode mode open/closed as requested by @EisenbergEffect
Updated passing settings from separated params to a object using object destructuring as discussed with @bigopon


Trying to make these changes I had a couple difficulties with my setup.

  1. vscode is not happy about adding typings to js files. How should I configure this correctly or use this?

I found a stack overflow page stating setting "javascript.validate.enable": false is the best option when using js + eslint; Someone also stated you should use ts files.

  1. I have an eslint extension installed but it does not seem to autofix with this repo. Should I do any specific configuration? (Previous question might be the reason that it is not formatting)

Autofix (at least for trailing spaces) seems to work with js validate set to false. I think we should add workspace settings to the project. (and a precommit hook would be awesome)

  1. To create these changes I tried to npm install my local dev branch of templating resources in a cleanly installed aurelia project. This completely failed. I had the same issues when testing aurelia-store recently.
    I ended up just copy/pasting dist files to be able to manually test. Is there any procedure how to create a local aurelia project using local deps for development?
// Error when using a local install (npm install ../aurelia/templating-resource)
aurelia-hide-style.js?7747:8 Uncaught (in promise) TypeError: aurelia_pal__WEBPACK_IMPORTED_MODULE_0__.DOM.injectStyles is not a function
    at injectAureliaHideStyleAtHead (aurelia-hide-style.js?7747:8)
    at Module.configure (aurelia-templating-resources.js?44e9:35)
    at eval (aurelia-framework.js?0682:201)
  1. After running gulp build everything in dist is shown as changed... I just ignored it for now but should the dist folder be added to the gitignore?
@bigopon

This comment has been minimized.

Copy link
Member

bigopon commented Mar 17, 2019

dist files are kind of weird case, I'm not sure what to do with it. It's kind of annoying for contributors sometimes. Maybe @fkleuver can config it right?

About (3), I'm not sure why, but I can have a look later.

@michaelw85

This comment has been minimized.

Copy link
Contributor Author

michaelw85 commented Mar 17, 2019

dist files are kind of weird case, I'm not sure what to do with it. It's kind of annoying for contributors sometimes. Maybe @fkleuver can config it right?

About (3), I'm not sure why, but I can have a look later.

I would assume a local npm install of package would work out of the box. I don't understand what's different compared to a package pulled down via npm.

michaelw85 added some commits Mar 17, 2019

@EisenbergEffect

This comment has been minimized.

Copy link
Member

EisenbergEffect commented Mar 17, 2019

Looks ready to merge. Any objections?

@fkleuver

This comment has been minimized.

Copy link
Member

fkleuver commented Mar 18, 2019

Unless there is time pressure to get this released, it'd be nicer to have this PR split up into one that fixes the workflow, and one that addresses this issue. That workflow stuff was WIP and would not bring things in a consistent state right now.

michaelw85 added some commits Mar 18, 2019

@michaelw85

This comment has been minimized.

Copy link
Contributor Author

michaelw85 commented Mar 18, 2019

@fkleuver Reverted merged CI build changes.

@EisenbergEffect EisenbergEffect merged commit 017befe into aurelia:master Mar 19, 2019

2 of 3 checks passed

ci/circleci: Build Error Your tests failed on CircleCI
Details
WIP ready for review
Details
license/cla Contributor License Agreement is signed.
Details

michaelw85 added a commit to michaelw85/documentation that referenced this pull request Apr 10, 2019

doc(shadow-dom, custom-elements): Add example
Add example and add optional options param.

Related: aurelia/templating-resources#368

@michaelw85 michaelw85 deleted the michaelw85:367_add_shadow_dom_attribute branch Apr 10, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.