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

Exports fixtures #6

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Exports fixtures #6

wants to merge 5 commits into from

Conversation

yofreke
Copy link
Contributor

@yofreke yofreke commented Aug 18, 2016

Chatted with @fairfieldt about this, for these exports we want to keep old functionality (even though it does not look like nice es6 code). It will probably be easier to fix the way things are exported once we are further along with this project (or even by hand on a per case basis).

(package.json was missing lodash)
I _think_ that the expected conversion is `exports = thing` -> `exports.default = thing`, which means that we will need to attach extra properties to the default, instead of exporting multiple named vars.  Need to check with @fairfieldt first
@juliankrispel
Copy link
Owner

@yofreke can I ask why?

@juliankrispel
Copy link
Owner

@yofreke @fairfieldt I'm not sure I understand why we should revert the part of the codemod for named exports.

  1. What is actually the problem with named exports and why is it hard to fix? (Maybe I'm missing something here)
  2. If we only transform default exports we end up using two module systems.
  3. Imo we should either transform or not. Transforming and warning the user will result in a difficult UX. We'd have to give the user a list of all the exports that we haven't touched... (seems overkill)

@juliankrispel
Copy link
Owner

ok, having read through the other pr more thoroughly I can see your reasoning...

However, it's expected that once we run the codemods that we'll discover more issues and iterate. I'm not entirely convinced it's worth blowing it off just yet, here's what I'd do to fix the issues:

  • replace exports member expressions (like exports.foo) with the nam of the export (foo)
  • throw errors if there are naming collisions
  • use let if export is mutated somewhere else in the file

I'm sure we run into more issues but this seems like a reasonable approach, what do you think?
I'm going to wait for your feedback before continuing @yofreke

@yofreke
Copy link
Contributor Author

yofreke commented Aug 19, 2016

Before we would do thing like export a class, and then add "static" methods to the class with exports.x = .... So if we replace exports = ... with export default ..., we should move all named exports to exports.default.<name> = ....

It might be a rather lengthy process finding every reference to a named export and updating it by hand. We thought in this one instance we would go with mod compatibility over (by hand) completeness. We did not actually look in to the validity of our feeling, so it may be the case that everything can easily be updated by hand.

It is okay if you want to hold off on this one until we have more info about how difficult by hand will be.

@juliankrispel
Copy link
Owner

Can you provide me with an example for that?

Julian Krispel-Samsel
rainforestqa.com
goodafternoon.co

On Aug 19, 2016, at 7:25 PM, Joe Brown notifications@github.com wrote:

Before we would do thing like export a class, and then add "static" methods to the class with exports.x = .... So if we replace exports = ... with export default ..., we should move all named exports to exports.default. = ....

It might be a rather lengthy process finding every reference to a named export and updating it by hand. We thought in this one instance we would go with mod compatibility over (by hand) completeness. We did not actually look in to the validity of our feeling, so it may be the case that everything can easily be updated by hand.

It is okay if you want to hold off on this one until we have more info about how difficult by hand will be.


You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or mute the thread.

@yofreke
Copy link
Contributor Author

yofreke commented Aug 19, 2016

This was the first file I tried d:
https://github.com/gameclosure/timestep/blob/master/src/AudioManager.js

exports = class AudioManager
exports.muteAll -> AudioManager.muteAll

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.

2 participants