Skip to content
This repository was archived by the owner on Aug 19, 2022. It is now read-only.

Upgrade to inline-style-prefixer v3 #945

Merged
merged 4 commits into from
Jan 8, 2018

Conversation

exogen
Copy link
Contributor

@exogen exogen commented Nov 29, 2017

This upgrades inline-style-prefixer to version 3. Fixes #918.

  • There is one failing test due to Prefixed properties should precede their unprefixed equivalents robinweser/inline-style-prefixer#147. If inline-style-prefixer does not incorporate this change eventually, we'll have to adjust its output ourselves. Thoughts?
  • Because inline-style-prefixer's default browser support has changed, and (per @alexlande) Radium roughly targets React's browser support, we now create a custom Prefixer for use with the prefixer plugin. The new Prefixer matches inline-style-prefixer v2's browser support, so people should be able to upgrade without breaking anything.
  • The browser data for the new Prefixer is generated by the new update-prefix-data script and stored in src/prefix-data (the script outputs ES source files).
  • New Flow interfaces were added for the new inline-style-prefixer modules we use.

@@ -176,7 +176,8 @@ rules:

brace-style: # enforce one true brace style
[2, "1tbs", { "allowSingleLine": true }]
camelcase: 2 # require camel case names
camelcase: # require camel case names, except in properties
[2, { "properties": "never" }]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

inline-style-prefixer's browser data generator outputs objects with keys like ios_saf. This disables the camelcase check for object properties. We can't easily add eslint-disable rules to those specific files because they're generated.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which files are generated?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

src/prefix-data/static.js and src/prefix-data/dynamic.js

Copy link
Member

@ryan-roemer ryan-roemer Nov 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there anything else in prefix-data? If not, can't we just revert this here and drop a new src/prefix-data/.eslintrc that overrides it there?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I considered that and would be happy to do it, lemme make the case for why I didn't though:

  • We'd need it in scripts too (the input object to generateData also has underscores)
  • Really I find the whole lint rule applying to properties extremely presumptuous. Other people's APIs use underscores (this example, JSON from REST APIs, even React has unstable_foo methods that people actually use) and it's lame needing an exception for that every single time. If our code were completely self-contained and didn't interact with anyone else's APIs then it would make sense.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 94.32% when pulling 5a2ba31 on chore/inline-style-prefixer-v3 into baa0ccc on master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 94.32% when pulling 5a2ba31 on chore/inline-style-prefixer-v3 into baa0ccc on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 94.32% when pulling 5a2ba31 on chore/inline-style-prefixer-v3 into baa0ccc on master.

* this if browser support changes or `inline-style-prefixer` gets fixes for the
* supported browsers.
*/
import path from 'path'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to lint the scripts.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. I changed ESLint to lint everything so this forgotten-directories thing stops happening (turns out test was also not linted).

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 94.32% when pulling 647e898 on chore/inline-style-prefixer-v3 into baa0ccc on master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 94.32% when pulling 647e898 on chore/inline-style-prefixer-v3 into baa0ccc on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 94.32% when pulling 647e898 on chore/inline-style-prefixer-v3 into baa0ccc on master.

@ryan-roemer
Copy link
Member

@exogen -- Looks like https://unpkg.com/[email protected]/ is released!

Do we just version bump to finish this PR or does other work remain?

/cc @alexlande

expect(rendered)
.to.contain('data-radium="true"').and
.to.contain('style="background:red;color:white"');
expect(rendered).to
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this an auto-formatted result? Chai assertions are typically always:

expect(foo)
  .to.SOMETHING.and
  .to.SOMETHING_ELSE.and
  .to.LAST_THING;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or is that prettier turned loose?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like that is what prettier wants. npm run lint will fail otherwise.

@stefvhuynh
Copy link
Contributor

looks like [email protected] has been released. according to the changelog (https://github.com/rofrischmann/inline-style-prefixer/blob/master/Changelog.md), the only changes are @exogen's PR and updated browser versions. seems like no other work needs to be done here?

@ryan-roemer
Copy link
Member

@stefvhuynh -- Give the update a go and see if you can get CI to pass!

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 94.32% when pulling 12ddeb7 on chore/inline-style-prefixer-v3 into baa0ccc on master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 94.32% when pulling 12ddeb7 on chore/inline-style-prefixer-v3 into baa0ccc on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 94.32% when pulling 12ddeb7 on chore/inline-style-prefixer-v3 into baa0ccc on master.

@coveralls
Copy link

coveralls commented Jan 2, 2018

Coverage Status

Coverage increased (+0.3%) to 94.32% when pulling cf6a094 on chore/inline-style-prefixer-v3 into baa0ccc on master.

Copy link
Contributor

@alexlande alexlande left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! 👍

@ryan-roemer
Copy link
Member

@alexlande -- Thanks!

@stefvhuynh -- You're a go for merge + publish!

@ryan-roemer ryan-roemer changed the title wip: Upgrade to inline-style-prefixer v3 Upgrade to inline-style-prefixer v3 Jan 4, 2018
@ryan-roemer
Copy link
Member

@stefvhuynh @alexlande @exogen -- What semver bump is this change?

@exogen
Copy link
Contributor Author

exogen commented Jan 4, 2018

@ryan-roemer Big dependency bumps like this are tricky – the intention here was to be MINOR (we tried to make sure to support the same browser versions as before, none of our tests had to change, we get some new i-s-p features). But then again i-s-p v3 was also a complete rewrite, it wouldn't surprise me if some edge cases were different for some people, so MAJOR would be the absolute safest.

@alexlande
Copy link
Contributor

Given that Radium is still on 0.x releases I think it would make sense to make this 0.20.0

@stefvhuynh stefvhuynh merged commit eecaa77 into master Jan 8, 2018
@stefvhuynh stefvhuynh deleted the chore/inline-style-prefixer-v3 branch January 8, 2018 20:45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[TRACKING] Update to use inline-style-prefixer v3
5 participants