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

fix(addStyles): support exports of transpiled transforms (options.transform) #333

Merged
merged 2 commits into from Oct 8, 2018

Conversation

akki-jat
Copy link
Contributor

What kind of change does this PR introduce?

bugfix

Did you add tests for your changes?
No

Solves #332

Does this PR introduce a breaking change?
No

@jsf-clabot
Copy link

jsf-clabot commented Jul 10, 2018

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Jul 10, 2018

Codecov Report

Merging #333 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #333   +/-   ##
=======================================
  Coverage   98.43%   98.43%           
=======================================
  Files           4        4           
  Lines          64       64           
  Branches       21       21           
=======================================
  Hits           63       63           
  Misses          1        1

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4217bd1...717601d. Read the comment docs.

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

Please add tests and we can merge this

@alexander-akait
Copy link
Member

Please accept CLA too

lib/addStyles.js Outdated
result = options.transform(obj.css);
result = typeof options.transform === 'function'
? options.transform(obj.css)
: Object.values(options.transform)[0](obj.css);
Copy link
Member

Choose a reason for hiding this comment

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

One question, why 0? Maybe better default?

Copy link
Contributor Author

@akki-jat akki-jat Aug 2, 2018

Choose a reason for hiding this comment

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

I first thought of options.transform.default(obj.css);
But then I thought if there are other modules which uses another name other than default, then Object.values(options.transform)[0](obj.css);, will support all of them.

Then decision is yours.

@alexander-akait
Copy link
Member

Error: Error: Uncaught [TypeError: Object.values is not a function], Object.values is not supported in old browsers

@akki-jat
Copy link
Contributor Author

akki-jat commented Aug 2, 2018

Yeah, you are right. So then we can use Object.keys or you suggest simply options.transform.default(obj.css);

@akki-jat
Copy link
Contributor Author

akki-jat commented Aug 2, 2018

@evilebottnawi I replaced Object.values with Object.keys to support old browsers.
But if you still prefer options.transform.default(obj.css); then tell me I will add that.

And as for accepting CLA, I already done that. I don't know why it is not reflecting here.

@akki-jat
Copy link
Contributor Author

akki-jat commented Aug 3, 2018

@evilebottnawi can you now take a look?

@michael-ciniawsky michael-ciniawsky changed the title fix(#332): Raise error when export default used instead of module.exp… fix(addStyles): raise an {Error} when export default is used (options.transform) Aug 7, 2018
Copy link
Member

@michael-ciniawsky michael-ciniawsky left a comment

Choose a reason for hiding this comment

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

Where does it currently check and throw within the loader :) ? While it's an valid issue it's debatable if adding a check for this to the loader in particular is really needed here , the style-loader is still kind of a node module (require) and node simply doesn't support ES Modules natively yet. What about adding an note to the README instead warning that one needs to transpile transforms written with ES Modules beforehand?

@akki-jat
Copy link
Contributor Author

akki-jat commented Aug 7, 2018

@michael-ciniawsky it's throw error: transform is not a function when export default or exports.default is used instead of module.exports.
Because when we use export default or exports.default, it returns the transform as object (not as a function) and actual transform function wrapped inside its default property

If want to check, I already created a repo. https://github.com/akki-jat/style-loader-bug--332

About the transpile, most of the user place their transform.js file outside the src folder and they default configuration which transpiles only src folder. And It would be irretating to user if we force this to them.

What do you think? Tell me, we will discuss it.

@michael-ciniawsky
Copy link
Member

michael-ciniawsky commented Aug 9, 2018

I see and I'm ok with using Object.keys to loop over the exports here so it at least throws correctly, but I still think there should be a note added explaining this and inform devs that in case their transform uses ES Modules they need to transpiled it beforehand

@michael-ciniawsky
Copy link
Member

michael-ciniawsky commented Aug 9, 2018

Around the transform option section something like e.g

README.md#transform

> :warning: In case your transform uses ES Module Syntax you need to transpile it, or otherwise...

@akki-jat
Copy link
Contributor Author

akki-jat commented Aug 9, 2018

@michael-ciniawsky you are right about the warning.
But what I am trying to say is right now even after the transpile it throws error.

Why throws error even after the transpile?

export default is transpiled in exports.default and it returns the transform as object (not as a function) and actual transform function wrapped inside its default property.

So, meaning this will solve problem that is occurring after transpiling.

@akki-jat akki-jat closed this Aug 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants