Add feature detection of unicode properties support #292

msamsel · 2019-07-03T07:47:17Z

Suggested merge commit message (convention)

Feature: Add feature detection of unicode properties support.

Additional information

Feature required for The enter should not be counted as a character ckeditor5#2574
Feature required for Replace feature detection of the unicode properties with the new one created in the utils module ckeditor5-mention#83

coveralls · 2019-07-03T07:52:20Z

Coverage increased (+0.0004%) to 99.826% when pulling ef16b13 on t/ckeditor5-word-count/5 into bacc764 on master.

mlewand · 2019-07-16T13:29:56Z

src/featuredetection.js

+ * @protected
+ * @namespace
+ */
+export default {


I was wondering whether it should be palced in https://github.com/ckeditor/ckeditor5-utils/blob/cf06347de71b235b564a13016a260af7c191a34d/src/unicode.js - but at the end the linked file is related to unicode handling, while this check is focused on RegExp in context of a given unicode goup handling.

However I'd give it better namespacing, I'm thinking more like: utils.featuredetection.regexp.isXyzSupported().

@mlewand to clarify:

import featureDetection from '@ckeditor5-utils/src/utils/featuredetection'; console.log( 'Regex group is supported:', featureDetection.regex.isUnicodePropertySupported() );

ps.: Shouldn't it be isUnicodePropertyEscapesSupported()? 😆

@msamsel please create also a PR in mention repo to use this util instaed own feature detection.

@jodator I was thinking more about:

import regExpFeatureDetection from '@ckeditor5-utils/src/utils/featuredetection/regexp'; console.log( 'RegExp group is supported:', regExpFeatureDetection.isUnicodePropertySupported() );

This would allow us to nicely organize stuff in future.

And it's important for us to use RegExp with p consistently, as this is how the type is named in JS.

RegExp with p consistently, a

typo, sorry :P

jodator

@mlewand one thing to clarify.
@msamsel use this util in mention also.

jodator · 2019-07-16T15:33:40Z

src/featuredetection.js

+ * @protected
+ * @namespace
+ */
+export default {


@mlewand to clarify:

import featureDetection from '@ckeditor5-utils/src/utils/featuredetection'; console.log( 'Regex group is supported:', featureDetection.regex.isUnicodePropertySupported() );

ps.: Shouldn't it be isUnicodePropertyEscapesSupported()? 😆

jodator · 2019-07-16T15:35:27Z

src/featuredetection.js

+ * @protected
+ * @namespace
+ */
+export default {


@msamsel please create also a PR in mention repo to use this util instaed own feature detection.

jodator · 2019-07-16T15:47:29Z

src/featuredetection.js

+export default {
+	/**
+	 * Indicates whether the current browser supports ES2018 Unicode properties like `\p{P}` or `\p{L}`.
+	 *


Since I'm always searching for this. Please add a link to:

https://www.unicode.org/reports/tr44/#GC_Values_Table

so we get a link to those categories :)

msamsel · 2019-07-17T09:48:40Z

ps.: Shouldn't it be isUnicodePropertyEscapesSupported()? 😆

@jodator usually in the documentation you can find the name "Unicode property" or just "property". The word "escape" seems to be strictly related to the existence of \ inside a specific regular expression. I think, in this case, the word "escape" should be skipped, as we don't talk about usage inside regular expression syntax, but rather accessibility of the browser's feature.

mlewand · 2019-07-17T10:36:39Z

Also let's try to keep the names simple if possible 🙂

jodator · 2019-07-17T10:46:16Z

OMG next time I'll use <joke> ... </joke> 😄

msamsel · 2019-07-17T12:48:31Z

OMG next time I'll use <joke> ... </joke> 😄

Those could be really useful, as it is really hard to notice the sarcasm in the written text, where you expect the feedback ;)

Reinmar · 2019-07-18T07:27:46Z

src/featuredetection/regexp.js

+		// See https://github.com/ckeditor/ckeditor5-mention/issues/44#issuecomment-487002174.
+
+		try {
+			isSupported = 'ć'.search( new RegExp( '[\\p{L}]', 'u' ) ) === 0;


Why new RegExp and not a literal?

@Reinmar I'm investigating this - ESLint is complaining about this.

Yeah, definitely there should be comments as this not clear in such cases :(

Tl;dr; ESLint doesn't recognize it.

Simillar discussion is here:
ckeditor/ckeditor5-word-count#10 (comment)

I mean that we have to bumpt the ESLint rules used in CKEditor to be able to use /\p{L}/u AFAICS.

OH we've already published the changes and after updating dependencies it works :) So our ESLint config works with this now 🎉

Ok so I tried it, and it's not working with our environment :(
Even when ESLint does not return an error, there is run acorn somewhere under the hood which throws a bunch of errors with those regexp literals:

There's an upcoming fix for that. Right now we cannot use them :(

OK, fair enough. Please leave a comment in the code with an explanation.

Reinmar · 2019-07-18T07:28:38Z

src/featuredetection/regexp.js

+ */
+
+/**
+ * @module utils/featuredetection/regexp


Does it have to be entire namespace for regexp? Do we expect to have many of those?

I think that moving this to env.js will be cleaner.

Something like this?

const env = { isMac: isMac( userAgent ), // ... features: { regexp: { isUnicodePropertySupported: isUnicodePropertySupported() } } // or just features: { isRegExpUnicodePropertySupported: isRegExpUnicodePropertySupported() } }

Does it have to be entire namespace for regexp? Do we expect to have many of those?

@Reinmar There are several features that are about to come (not widely supported) into JS RegExp:

s (a.k.a. dotAll) flag

named groups

Unicode property escapes (that's in this PR)

Also ES2018 finally added lookahead/lookbehind, luckily both seems to be implemented by all major browser vendors. 🎉

Note that future ES revisions might also expose more RegExp features to further reduce the gap.

There are several features that are about to come (not widely supported) into JS RegExp: (..)

I wonder how many of them are we going to use and will have to detect.

For me having feature detection inside env.js seems to be a good solution. The first example is more appealing to me.

@mlewand What would be the problem if we had like 5 regexp-oriented properties in env?

We can create new namespaces for every new distinctive thing that may become "plural" with time. But many of them don't become plural and majority doesn't exceed 3 options. If you don't see technical problems for keeping that in env, I'd keep that in env. They will be easier to find there.

@jodator

// or just features: { isRegExpUnicodePropertySupported: isRegExpUnicodePropertySupported() }

sounds best to me – it's the most flexible option.

Reinmar

See comments.

jodator · 2019-07-19T14:51:07Z

@msamsel The one thing remains - move the feature detection to the env as

features: {
		isRegExpUnicodePropertySupported: isRegExpUnicodePropertySupported()
	}

I'd keep that in env. They will be easier to find there.

msamsel · 2019-07-22T08:18:28Z

@jodator I've moved feature detection to env.

ma2ciek · 2019-07-22T16:47:23Z

features: {
isRegExpUnicodePropertySupported: isRegExpUnicodePropertySupported()
}

This looks a little bit inconsistent for me, sorry for bumping late into the established discussion.

Shouldn't it be like the following? AFAIK we don't prefix boolean props with is, has, etc.

features: {
	regExpUnicodePropertySupported: isRegExpUnicodePropertySupported()
}

jodator · 2019-07-23T08:42:39Z

Shouldn't it be like the following? AFAIK we don't prefix boolean props with is, has, etc.

@ma2ciek dunno, it looks like we do :P

ckeditor5-utils/src/env.js

Line 26 in 21f1c4e

isMac: isMac( userAgent ),

ckeditor5-utils/src/env.js

Line 34 in 21f1c4e

isEdge: isEdge( userAgent ),

ckeditor5-utils/src/env.js

Line 42 in 21f1c4e

isGecko: isGecko( userAgent ),

from other files:
https://github.com/ckeditor/ckeditor5-engine/blob/9a40550a7425670264974edc9252f3dcf888ce2f/src/model/element.js#L87

https://github.com/ckeditor/ckeditor5-engine/blob/a58265d1c76c1e2345772861b05770280af13297/src/model/documentselection.js#L680

So we actually prefix boolean values with is/has/are :)

ma2ciek · 2019-07-23T08:57:06Z

Shouldn't it be like the following? AFAIK we don't prefix boolean props with is, has, etc.

@ma2ciek dunno, it looks like we do :P

Wow. I was pretty sure we do the opposite. So let's be consistent.

PS OTOH I don't like this convention because you end up not knowing if the property has a call signature or not.

jodator · 2019-07-23T09:04:47Z

I'm merging this one as other PRs might be closed later on and this will be already used by Mention feature.

msamsel mentioned this pull request Jul 3, 2019

Remove enter from counted characters ckeditor/ckeditor5-word-count#10

Merged

mlewand reviewed Jul 16, 2019

View reviewed changes

jodator reviewed Jul 16, 2019

View reviewed changes

Add feature detection utils with unicode properties support.

118c677

Move feature detection to more organized structure.

e916e1d

msamsel force-pushed the t/ckeditor5-word-count/5 branch from ef16b13 to e916e1d Compare July 17, 2019 10:25

msamsel mentioned this pull request Jul 17, 2019

Replace feature detection of the unicode properties with the new one created in the utils module ckeditor/ckeditor5-mention#83

Merged

msamsel requested review from jodator and mlewand July 17, 2019 12:46

Reinmar reviewed Jul 18, 2019

View reviewed changes

Reinmar suggested changes Jul 18, 2019

View reviewed changes

jodator mentioned this pull request Jul 18, 2019

Add ECMA version option to acorn parser options. ckeditor/ckeditor5-dev#535

Merged

Mateusz Samsel added 2 commits July 18, 2019 12:00

Add comment to regular expression.

4053957

Add referrence in code to issue number.

c496fbb

msamsel requested a review from Reinmar July 18, 2019 10:32

Move feature detection to env.js. Move unit test with them.

0e324ed

Update isRegExpUnicodePropertySupported() docs.

22001a0

jodator merged commit 21c0f6b into master Jul 23, 2019

jodator deleted the t/ckeditor5-word-count/5 branch July 23, 2019 09:05

jodator mentioned this pull request May 18, 2020

Add ecmaVersion to acorn configuration ckeditor/ckeditor5#7169

Closed

Add feature detection of unicode properties support #292

Add feature detection of unicode properties support #292

Conversation

msamsel commented Jul 3, 2019 • edited by pomek Loading

Suggested merge commit message (convention)

Additional information

coveralls commented Jul 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jodator left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msamsel commented Jul 17, 2019 • edited Loading

mlewand commented Jul 17, 2019 • edited Loading

jodator commented Jul 17, 2019

msamsel commented Jul 17, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msamsel Jul 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msamsel Jul 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Reinmar left a comment

Choose a reason for hiding this comment

jodator commented Jul 19, 2019

msamsel commented Jul 22, 2019

ma2ciek commented Jul 22, 2019

jodator commented Jul 23, 2019

ma2ciek commented Jul 23, 2019

jodator commented Jul 23, 2019

msamsel commented Jul 3, 2019 •

edited by pomek

Loading

msamsel commented Jul 17, 2019 •

edited

Loading

mlewand commented Jul 17, 2019 •

edited

Loading

msamsel commented Jul 17, 2019 •

edited

Loading

msamsel Jul 18, 2019 •

edited

Loading

msamsel Jul 18, 2019 •

edited

Loading