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

Should not magically convert kebab-cased values to camelCased values #13

Merged
merged 7 commits into from
Nov 7, 2018

Conversation

StarpTech
Copy link
Collaborator

@StarpTech StarpTech commented Oct 11, 2018

Failing tests to reproduce will fix #12

@wooorm
Copy link
Owner

wooorm commented Oct 11, 2018

Wait, could you provide us with some more info?

@StarpTech
Copy link
Collaborator Author

StarpTech commented Oct 11, 2018

Please ask any specific question. The current implementation handles an attribute with the name class-name as class property because after normalization the delimiter is removed and transformed to lowercase. This PR will fix this situation. I don't know if it's 100% correct but all tests are passed.

@StarpTech
Copy link
Collaborator Author

@wooorm is it clear now?

@wooorm
Copy link
Owner

wooorm commented Oct 12, 2018

Ah, missed that. Thanks Dustin!

I feel this is similar to #11 (comment). It feels like you want something that never normalises values, whereas this project is meant for the normalising part 🤔

@StarpTech
Copy link
Collaborator Author

StarpTech commented Oct 12, 2018

I feel this is similar to #11 (comment). It feels like you want something that never normalises values, whereas this project is meant for the normalising part

No, it's not related because we will always lowercase the attributes. I'm fine with normalization but my idea in #11 was to respect case-sensitivity when the known attribute isn't supported by the tag.

According to this issue can we merge and release it?

@wooorm
Copy link
Owner

wooorm commented Oct 13, 2018

@StarpTech The : and - stripping is I believe pretty important, if you remove it, everything breaks: SVG, XML, XLink, and XMLNS completely, accept-charset / http-equiv in HTML. 🤔

Could you try adding some tests showing they don’t break?

@StarpTech
Copy link
Collaborator Author

StarpTech commented Oct 13, 2018

There are already tests for -, : Could you tell me a specific example?
It works because the normal property of the schema maintains a list of normalized + unnormalized values:

{ xmllang: 'xmlLang',
  'xml:lang': 'xmlLang',
  xmlbase: 'xmlBase',
  'xml:base': 'xmlBase',
  xmlspace: 'xmlSpace',
  'xml:space': 'xmlSpace',
  xlinkactuate: 'xLinkActuate',
  'xlink:actuate': 'xLinkActuate',
  xlinkarcrole: 'xLinkArcRole',
  'xlink:arcrole': 'xLinkArcRole',
  xlinkhref: 'xLinkHref',
  'xlink:href': 'xLinkHref',
  xlinkrole: 'xLinkRole',
  'xlink:role': 'xLinkRole',
  xlinkshow: 'xLinkShow',
  'xlink:show': 'xLinkShow',
  xlinktitle: 'xLinkTitle',
  'xlink:title': 'xLinkTitle',
  xlinktype: 'xLinkType',
  'xlink:type': 'xLinkType',

and when the attribute is defined it can be just returned https://github.com/wooorm/property-information/blob/master/find.js#L21

@wooorm
Copy link
Owner

wooorm commented Oct 13, 2018

@StarpTech Ahh I forgot about that. Sorry!

Well then I’m fine with this change, as long as it’s properly documented and tested, and in a major release!

@StarpTech
Copy link
Collaborator Author

@wooorm doc was updated!

@StarpTech
Copy link
Collaborator Author

@wooorm I'm not aware of any additional test-scenario. If you find something ping me. It would be great when we can merge it.

@StarpTech
Copy link
Collaborator Author

@wooorm ping.

@StarpTech
Copy link
Collaborator Author

@wooorm this test will fail https://github.com/syntax-tree/hastscript/blob/master/test.js#L148. It seems that the current behaviour was intentional?

@wooorm
Copy link
Owner

wooorm commented Oct 20, 2018

@StarpTech sorry for the delay!

Yes, it was intentional to be chill about input, but maybe that’s not good.
Anyway, this will be a breaking change, so I’m a bit hesitant in doing a major release of all the rehype and hast packages!

@wooorm wooorm changed the title add failing tests Should not magically convert kebab-cased values to camelCased values Oct 20, 2018
@StarpTech
Copy link
Collaborator Author

StarpTech commented Oct 20, 2018

@wooorm I think this change will definitely improve the handling because as you see this issue comes from real-world user experience 😄

@StarpTech
Copy link
Collaborator Author

Yes, it was intentional to be chill about input, but maybe that’s not good.
Anyway, this will be a breaking change, so I’m a bit hesitant in doing a major release of all the rehype and hast packages!

You don't agree with the change or what's the problem?

@wooorm
Copy link
Owner

wooorm commented Oct 23, 2018

@StarpTech I agree with the change, just don’t want to do major releases too often if they‘re only tiny changes (but still breaking)

@StarpTech
Copy link
Collaborator Author

StarpTech commented Oct 23, 2018

Just as a reference: Here is another user which use the attribute item-type but due to the normlization process it is transformed to itemtype

When can we expect a major release? What's wrong with the circumstance that not every package is up-to-date when it's from your side, not a critical issue?

@StarpTech
Copy link
Collaborator Author

@wooorm would be great to have a decision 🤔 so that I can think about a different solution for my project.

@StarpTech
Copy link
Collaborator Author

@wooorm ping!

@wooorm wooorm merged commit 51c678d into wooorm:master Nov 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

should not handle class-name as class property
2 participants