Ready for React 16 #76

diligiant · 2017-08-14T21:55:34Z

This code now works with React 15 and React 16 (library and example). Tests will have to wait that enzyme is made compatible with React 16 to work.

3 versions of the library are now available: cjs, umd & esm.

Removed deprecated React code, switched examples to webpack, enforced AirBnb lint, added Replace Elements test. Signed-off-by: Frédéric Miserey <[email protected]>

Signed-off-by: Frédéric Miserey <[email protected]>

rxaviers · 2017-08-15T00:29:26Z

🎉 a lot of great updates @diligiant, thanks. One comment for now: can we not include dist? I'd like to avoid the need to update this file during development. Let's only include this on releases...

An extraneous <span> was surrounding a replacement ending with a dual <span> for nothing. Signed-off-by: Frédéric Miserey <[email protected]>

or else simple FormattedText was output as individual characters (`“t””h””i””s”`). Added some test to prevent this as well as ensuring the proper structure of Replaced Elements. Borrowed alwaysArray from Globalizejs. Signed-off-by: Frédéric Miserey <[email protected]>

diligiant · 2017-08-15T15:07:53Z

@rxaviers ok, will do. So basically, I leave you to include "dist" ? Ok boss ;)

Elements substitution was incomplete when multiple elements were involved : If multiple substitutions of the same element occured when the format had been already splitted into multiple nodes, the last substitution(s) wouldn’t be applied as the last segment(s) wouldn’t be inspected. ForEach evaluated the length of the array at once and the array was expanded during the substitutions. The order of the elements in the new test is important: first [br] splits the format node into a 3-node array then [strong] should sustitute nodes to the first and last nodes. Renamed a few variables as well to enhance readability. Signed-off-by: Frédéric Miserey <[email protected]>

replaced for/forEach by map and flapMap. getElement param in spreadElemetsInBetweemItems is now always a function. Signed-off-by: Frédéric Miserey <[email protected]>

rxaviers · 2017-09-02T17:43:20Z

examples/package.json

+    "babel-plugin-transform-class-properties": "^6.24.1",
+    "babel-preset-react": "^6.24.1",
+    "imports-loader": "^0.7.1",
+    "webpack": "^3.5.4"
  },


Let's add react ^16.0.0 in devDeps..

I'm really not good at this! Isn't this a problem if deps are still ^15?

Yeap, we should loose that in dev by adding the || 16 :)

rxaviers · 2017-09-02T17:43:39Z

examples/package.json

+    "cldr-data": ">=31",
+    "globalize": "^1.3.0",
+    "react": "^15.6.0",
+    "react-dom": "^15.6.1"


Shall we add a || ^16.0.0?

You're right. Damned ;) I also forgot to add my new fav example with markup ;)

rxaviers · 2017-09-02T17:44:23Z

examples/package.json

  },
  "scripts": {
-    "build": "mkdir -p react-globalize && cp ../dist/*.js react-globalize/ && browserify --debug --transform reactify index.js > app.js"
+    "build": "mkdir -p react-globalize && cp ../dist/*.umd.js react-globalize/index.js && webpack"


rxaviers · 2017-09-02T17:47:32Z

package.json

+    "test": "mocha test",
+    "test:debug": "node ./node_modules/.bin/mocha --inspect-brk test",
+    "lint": "eslint rollup.config.js src test",
+    "pretest": "npm run lint"


🎉 Yay! No more grunt.

rxaviers · 2017-09-02T17:57:08Z

src/message.js


-                if (typeof str !== "string") {
-                    return; // continue;
+            for (let i = 0; i < nodes.length; i += 1) {


Great catch!

ok. You'll notice I got rid of this ugly imperative coding in the next commit. Way safer.

diligiant · 2017-09-02T18:07:02Z

Even if I switched to a safe flatMap, it might be nice to explain as it wasn’t done “just because” !! WDYT?

…

On 2 Sep 2017 at 20:03 +0200, Rafael Xavier de Souza ***@***.***>, wrote: @rxaviers commented on this pull request. In src/message.js: > - if (typeof str !== "string") { - return; // continue; + for (let i = 0; i < nodes.length; i += 1) { Ignore my message asking to include a comment... This code has been modified anyway in subsequent commits. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

rxaviers

Great job! Your changes LGTM.

Having said that, we definitely need to improve our tests in here, for example, assert element replacement in messages and others.

We should also move the default message approach into globalize itself. globalizejs/globalize#575

We could merge this and do those as later steps...

Thanks again

rxaviers · 2017-09-02T18:15:41Z

Let's give some time in case @necolas and @kborchers have any input... Otherwise, let's merge.

rxaviers · 2017-09-02T18:22:40Z

Even if I switched to a safe flatMap, it might be nice to explain as it wasn’t done “just because” !! WDYT?

👍, a brief explanation as an internal comment is definitely welcome. Also, it would be wise to include a test case to assert this doesn't regress in the future.

Signed-off-by: Frédéric Miserey <[email protected]>

diligiant · 2017-09-03T13:31:55Z

@rxaviers the test case is there (line 81 of test/formatMessage.spec.js)
[strong][/strong] [br/] [strong][/strong]
with br first in the elements parameter)

diligiant · 2017-09-03T13:36:15Z

We should also move the default message approach into globalize itself. globalizejs/globalize#575

Do you want to handle this or may I?

May you also take a look at #75 as it's the same area ; the purpose is to have a more predictable and logical behavior (and consistent with "normal" globalize).

Signed-off-by: Frédéric Miserey <[email protected]>

necolas · 2017-10-02T17:26:20Z

Which part of this patch is addressing React 16 compatibility? Can that be broken out into a separate PR so we can merge the React upgrade fix?

diligiant · 2017-10-03T13:40:32Z

@necolas all of it unless I'm mistaken!

rxaviers · 2017-10-03T14:06:45Z

In this PR, along with React 16 upgrades (e.g., React.DOM.span -> React.createElement), there are its needed test upgrades (Enzyme 3), plus tooling and code modernization too. @diligiant can correct me if needed...

Can that be broken out into a separate PR so we can merge the React upgrade fix?

@necolas please are you looking for safety (prevent regressions) or speed (get this PR merged quickly)? I think at this point it will be hard to cherry-pick parts without having to deal with lots of conflicts, but it should be straightforward to merge this work into an upcoming 1.0.0 release according to #77 option (a).

What do you think?

necolas · 2017-10-03T17:02:09Z

are you looking for safety or speed

Both. This PR has been open for a while and has a lot of unrelated changed.

paularmstrong · 2017-10-05T20:31:57Z

I opened #78 to handle only changes to make React 16 work and avoid all of the syntax and build system changes

rxaviers · 2017-10-05T21:06:20Z

@paularmstrong thanks for creating #78. I also obviously prefer atomic PRs.

Having said, we need to (a) assert if #78 is sufficient and (b) I don't think @diligiant work should be lost. The code and build improvements are welcome. @diligiant can you try to rebase your changes on top of #78? How much does it conflict?

diligiant · 2017-10-05T21:08:30Z

merge #78 and I’ll follow-up.

Ref #76 Ref #79

Ref #76 Ref #80 Signed-off-by: Frédéric Miserey <[email protected]>

Ref #76 Ref #80

Ref #76 Ref #80 Signed-off-by: Frédéric Miserey <[email protected]>

Ref #76 Ref #80

Ref #76

or else simple FormattedText was output as individual characters (`“t””h””i””s”`). Added some test to prevent this as well as ensuring the proper structure of Replaced Elements. Borrowed alwaysArray from Globalizejs. Ref #76 Signed-off-by: Frédéric Miserey <[email protected]>

Elements substitution was incomplete when multiple elements were involved : If multiple substitutions of the same element occured when the format had been already splitted into multiple nodes, the last substitution(s) wouldn’t be applied as the last segment(s) wouldn’t be inspected. ForEach evaluated the length of the array at once and the array was expanded during the substitutions. The order of the elements in the new test is important: first [br] splits the format node into a 3-node array then [strong] should sustitute nodes to the first and last nodes. Renamed a few variables as well to enhance readability. Ref #76 Signed-off-by: Frédéric Miserey <[email protected]>

Ref globalizejs#76

or else simple FormattedText was output as individual characters (`“t””h””i””s”`). Added some test to prevent this as well as ensuring the proper structure of Replaced Elements. Borrowed alwaysArray from Globalizejs. Ref globalizejs#76 Signed-off-by: Frédéric Miserey <[email protected]>

Elements substitution was incomplete when multiple elements were involved : If multiple substitutions of the same element occured when the format had been already splitted into multiple nodes, the last substitution(s) wouldn’t be applied as the last segment(s) wouldn’t be inspected. ForEach evaluated the length of the array at once and the array was expanded during the substitutions. The order of the elements in the new test is important: first [br] splits the format node into a 3-node array then [strong] should sustitute nodes to the first and last nodes. Renamed a few variables as well to enhance readability. Ref globalizejs#76 Signed-off-by: Frédéric Miserey <[email protected]>

Ref globalizejs#76

Frédéric Miserey added 4 commits August 10, 2017 12:42

switch to rollup and generate 3 dist

78fde30

accomodate modern js

fa21635

end of cleanup

56e3fe2

Removed deprecated React code, switched examples to webpack, enforced AirBnb lint, added Replace Elements test. Signed-off-by: Frédéric Miserey <[email protected]>

package files cleanup

851a91f

Signed-off-by: Frédéric Miserey <[email protected]>

Frédéric Miserey added 2 commits August 15, 2017 13:53

Much leaner Elements replacement

966763f

An extraneous <span> was surrounding a replacement ending with a dual <span> for nothing. Signed-off-by: Frédéric Miserey <[email protected]>

Frédéric Miserey added 2 commits September 2, 2017 00:57

cleaner and safer version using fp

7d51259

replaced for/forEach by map and flapMap. getElement param in spreadElemetsInBetweemItems is now always a function. Signed-off-by: Frédéric Miserey <[email protected]>

rxaviers reviewed Sep 2, 2017

View reviewed changes

rxaviers approved these changes Sep 2, 2017

View reviewed changes

Frédéric Miserey added 3 commits September 2, 2017 20:31

works with react 15 and 16

50672b3

Signed-off-by: Frédéric Miserey <[email protected]>

Add a genuine use case for Element Replacement

85cab85

Signed-off-by: Frédéric Miserey <[email protected]>

comment to explain the rational of flatMap vs forEach

af95358

Signed-off-by: Frédéric Miserey <[email protected]>

Frédéric Miserey added 6 commits September 5, 2017 12:08

adject react dependencies

87977c4

Signed-off-by: Frédéric Miserey <[email protected]>

Enzyme 3.0 updates

6b9f646

Signed-off-by: Frédéric Miserey <[email protected]>

rollup .50 syntax changes

8b4d0ee

Signed-off-by: Frédéric Miserey <[email protected]>

bumped versions to react-16, enzyme-3 & rollup 0.50

2d931c9

Signed-off-by: Frédéric Miserey <[email protected]>

genuine version bump

5222e0f

Signed-off-by: Frédéric Miserey <[email protected]>

React-16 has been released!

967e828

Signed-off-by: Frédéric Miserey <[email protected]>

paularmstrong mentioned this pull request Oct 5, 2017

Minimum changes to support React 16 #78

Closed

paularmstrong mentioned this pull request Oct 6, 2017

Use rollup for build system #79

Closed

rxaviers pushed a commit that referenced this pull request Oct 8, 2017

Use rollup for build system (1/2)

8d92b0f

Ref #76 Ref #79

diligiant closed this Oct 8, 2017

diligiant deleted the react-16 branch October 8, 2017 21:04

rxaviers mentioned this pull request Oct 11, 2017

Upgrade to support React 16 #80

Closed

rxaviers pushed a commit that referenced this pull request Oct 17, 2017

Upgrade React to v16 (2/2)

e459ebd

Ref #76 Ref #80 Signed-off-by: Frédéric Miserey <[email protected]>

rxaviers pushed a commit that referenced this pull request Oct 17, 2017

Upgrade React to v16 (1/2)

451e30f

Ref #76 Ref #80

rxaviers pushed a commit that referenced this pull request Oct 17, 2017

Upgrade React to v16 (1/2)

457efe2

Ref #76 Ref #80 Signed-off-by: Frédéric Miserey <[email protected]>

rxaviers pushed a commit that referenced this pull request Oct 17, 2017

Upgrade React to v16 (2/2)

163fbd1

Ref #76 Ref #80

rxaviers pushed a commit that referenced this pull request Oct 17, 2017

Message: Add replace elements test

d5e1eac

Ref #76

rxaviers pushed a commit to rxaviers/react-globalize that referenced this pull request Oct 17, 2017

Message: Add replace elements test

6af11a3

Ref globalizejs#76

rxaviers added a commit to rxaviers/react-globalize that referenced this pull request Oct 17, 2017

Message: Test adjustments for [email protected]

50238dd

Ref globalizejs#76

rxaviers pushed a commit to rxaviers/react-globalize that referenced this pull request Oct 17, 2017

Examples: Update tooling and modernize code

a4f1864

Ref globalizejs#76

rxaviers mentioned this pull request Oct 17, 2017

Examples: Update tooling and modernize code #85

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ready for React 16 #76

Ready for React 16 #76

diligiant commented Aug 14, 2017

rxaviers commented Aug 15, 2017

diligiant commented Aug 15, 2017

rxaviers Sep 2, 2017

diligiant Sep 2, 2017

rxaviers Sep 2, 2017

rxaviers Sep 2, 2017

diligiant Sep 2, 2017

rxaviers Sep 2, 2017

rxaviers Sep 2, 2017

rxaviers Sep 2, 2017 •

edited

Loading

diligiant Sep 2, 2017

diligiant commented Sep 2, 2017 via email

rxaviers left a comment •

edited

Loading

rxaviers commented Sep 2, 2017

rxaviers commented Sep 2, 2017

diligiant commented Sep 3, 2017

diligiant commented Sep 3, 2017

necolas commented Oct 2, 2017

diligiant commented Oct 3, 2017

rxaviers commented Oct 3, 2017

necolas commented Oct 3, 2017

paularmstrong commented Oct 5, 2017

rxaviers commented Oct 5, 2017

diligiant commented Oct 5, 2017

Ready for React 16 #76

Ready for React 16 #76

Conversation

diligiant commented Aug 14, 2017

rxaviers commented Aug 15, 2017

diligiant commented Aug 15, 2017

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

rxaviers Sep 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diligiant commented Sep 2, 2017 via email

rxaviers left a comment • edited Loading

Choose a reason for hiding this comment

rxaviers commented Sep 2, 2017

rxaviers commented Sep 2, 2017

diligiant commented Sep 3, 2017

diligiant commented Sep 3, 2017

necolas commented Oct 2, 2017

diligiant commented Oct 3, 2017

rxaviers commented Oct 3, 2017

necolas commented Oct 3, 2017

paularmstrong commented Oct 5, 2017

rxaviers commented Oct 5, 2017

diligiant commented Oct 5, 2017

rxaviers Sep 2, 2017 •

edited

Loading

rxaviers left a comment •

edited

Loading