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

Conversation

@aminya
Copy link
Contributor

@aminya aminya commented Jul 4, 2020

Description of the Change

This pull requests updates underscore-plus without changing the API:

  • decaffeinate: src code is converted to Modern JavaScript
  • Rollup: underscore-plus is built with rollup which makes it load much faster
  • Tree shaking: now underscore-plus is tree shakable. It uses named exports which allows other tools like Rollup to tree-shake it, similar to the original underscore
  • babel: uses babel for conversion to cjs
  • Enhances the performance and loading time (in the next PRs I will continue this)

Verification Process

Tests are untouched to show backward compatibility! As you see all of them pass.

Applicable Issues

Release Notes

  • decaffeinate: src code is converted to Modern JavaScript
  • Rollup: underscore-plus is built with rollup which makes it load much faster
  • Tree shaking: now underscore-plus is tree shakable. It uses named exports which allows other tools like Rollup to tree-shake it, similar to the original underscore
  • babel: uses babel for conversion to cjs
  • Enhances the performance and loading time (in the next PRs I will continue this)

@aminya aminya marked this pull request as draft July 4, 2020 13:24
@aminya aminya force-pushed the modern branch 2 times, most recently from 1bf9419 to c4d9999 Compare July 4, 2020 13:37
@aminya aminya changed the title Modernize (decaffeinate + treeshaking) Modernize (decaffeinate + rollup) Jul 4, 2020
@aminya aminya marked this pull request as ready for review July 4, 2020 14:08
@aminya
Copy link
Contributor Author

aminya commented Jul 4, 2020

@lkashef I updated underscore-plus to be faster. 😉

@aminya aminya force-pushed the modern branch 4 times, most recently from 951ff97 to ce2d6ef Compare July 11, 2020 10:15
@lkashef
Copy link

lkashef commented Jul 11, 2020

Hi @aminya, I think the main purpose here is decaffeination, so I am wondering if the performance enhancement could work on their own for ease of review and also following this template that is more relevant to performance enhancement PRs

@aminya
Copy link
Contributor Author

aminya commented Jul 12, 2020

Hi @lkashef!

Yes, I did not change any source code here. The performance improvement is a side effect of decaffeination! To be able to use import/export and optional chaining (to keep it near to CoffeeScript) I hade to use Rollup and Babel. I did not change any source coded in this pull request, this all is just to make the automatic decaffeination nicer. These changes start from this commit

Commit-messages show this as well. Each commit-message is like:

name of the function + the change made to make automatic decaffeination nicer

I have kept my performance improvement ideas for separate pull requests. Let me know if you have questions, I can explain about each commit if it is needed.

@Aerijo
Copy link

Aerijo commented Jul 17, 2020

@aminya There are decaffeinating tools like this that will handle translating things like optional chaining. Introducing a Rollup dependency can be split into a separate PR, as can the other script / CI changes.

@aminya
Copy link
Contributor Author

aminya commented Jul 17, 2020

@aminya There are decaffeinating tools like this that will handle translating things like optional chaining.

Hi @Aerijo!

This pull request is already done. I added optional chaining support to decaffeinate myself in this pull request 😄 I used the same thing.

Introducing a Rollup dependency can be split into a separate PR, as can the other script / CI changes.

Rollup allows using import ... from ... and export * from. That is necessary for the functionality of this pull request. Otherwise, this will not work. The changes related to Rollup are minimal. They are in these two commits:
ce2a05e
f69f260

@Aerijo
Copy link

Aerijo commented Jul 17, 2020

I'm not sure I follow; are you saying the current CoffeeScript code cannot be translated into correct JS code? Or just that you want to use a specific import syntax, etc., which is not supported yet without transpiling? If the latter, my point above is that this can be split into multiple steps: decaffeinate, and then introduce Rollup, etc. Decaffeinating in and of itself is a straightforward change, but if you tie it with the other changes then it can't be accepted unless the new dependencies are also accepted.

@aminya
Copy link
Contributor Author

aminya commented Jul 17, 2020

are you saying the current CoffeeScript code cannot be translated into correct JS code?

No. I said this is already translated.

Or just that you want to use a specific import syntax, etc., which is not supported yet without transpiling? If the latter, my point above is that this can be split into multiple steps: decaffeinate, and then introduce Rollup, etc.

I want to use import-export, and without that, the transformation should be done using babel instead of rollup which generates a slow code. I used rollup here for import/export transformation, which transforms those without any runtime cost. If separating these two commits is what you want, I do not mind separating them, but that will have a performance regression for the current PR.

@Aerijo
Copy link

Aerijo commented Jul 17, 2020

I do not mind separating them, but that will have a performance regression for the current PR.

Only because you've bundled a performance enhancement in this PR though, right? When compared to the library's current state there should be no significant impact.

Adding a PR to enhance performance is great, I don't have an opinion one way or another on it (though I'm not responsible for reviewing and merging one). But like what @lkashef mentioned, decaffeinating is already a big (if straightforward) change in and of itself, so would benefit from having it's own PR. And to be clear, when I say decaf, I mean into "plain" JS, without introducing Rollup or Babel. You can always make a followup PR to introduce the import ... syntax using one of those tools.

@aminya aminya mentioned this pull request Jul 17, 2020
@aminya
Copy link
Contributor Author

aminya commented Jul 17, 2020

The decaffeinate subset of this pull request is created here: #22

@lkashef lkashef mentioned this pull request Jul 17, 2020
8 tasks
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.

3 participants