Added migrators for w-* attrs & code restructure #1190
Conversation
update from master
Update from master
…migrate-w-attr
There was a problem hiding this comment.
This is really awesome!
I left a few comments. Also we still need to remove the deprecated features from the codebase (outside of the migrate Taglib). Eg: w-preserve should be removed from https://github.com/marko-js/marko/blob/master/src/components/taglib/components-transformer.js#L70
| values.split(",").forEach(val => { | ||
| const attrValue = el.getAttributeValue(val); | ||
| if (attrValue) | ||
| el.setAttributeValue(`${val}:no-update`, attrValue); |
There was a problem hiding this comment.
In the else here we should still add ${val}:no-update with an empty attribute since the users intent may be to have the attribute be unset during the initial render and then mutated with JS later.
| `The "${name}" attribute is deprecated. Please use "${newAttrName}" attribute instead. See: https://github.com/marko-js/marko/wiki/Deprecation:-w-*-Atrributes` | ||
| ); | ||
|
|
||
| attr.name = newAttrName; |
There was a problem hiding this comment.
I could be wrong but I think the if variants have their attribute values as a string and we will need to parse it as javascript a node.
Codecov Report
@@ Coverage Diff @@
## master #1190 +/- ##
==========================================
- Coverage 90.62% 90.57% -0.05%
==========================================
Files 321 328 +7
Lines 12209 12265 +56
==========================================
+ Hits 11064 11109 +45
- Misses 1145 1156 +11
Continue to review full report at Codecov.
|
update from master
…migrate-w-attr
| const templateRoot = el.parentNode; | ||
| const walker = context.createWalker({ | ||
| enter(node) { | ||
| if (node.type == "HtmlElement" && node.hasAttribute) { |
There was a problem hiding this comment.
Looks good but I think that we also need to check for type === "CustomTag". It would be simplest to only check for node.hasAttribute as only HTMLElement's and CustomTags's should have that method.
| @@ -0,0 +1,16 @@ | |||
| module.exports = function addIdScopedAttr(context, el, attrValue) { | |||
| const templateRoot = el.parentNode; | |||
There was a problem hiding this comment.
el.parentNode is not the actual template root. We actually want to use context.root.
There was a problem hiding this comment.
At this point, context.root is undefined. So took el.parentNode
| const commonMigrators = fs | ||
| .readdirSync(path.join(__dirname, "./common/")) | ||
| .map(dir => require(`./common/${dir}`)); | ||
| const getMigrators = d => |
There was a problem hiding this comment.
I think we should revert this such that the common migrations is a flat structure (move all the w- stuff up a level).
As it stands right now all of these are in an event-handlers folder which is not totally accurate and I think unnecessary.
There was a problem hiding this comment.
Cool. Let me structure it.
|
Looks good, thanks for all the work on this 😄! |
|
Glad to be of help! :D |
Description
Added migrators for
w-id→keyw-for&for-key→for:scoped:key→:scopedw-preserve→no-updatew-preserve-attrs→:no-updateRestructured the code in taglibs/migrate
Checklist: