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

Feature/tree shaking #29

Merged
merged 31 commits into from
Jul 18, 2020
Merged

Feature/tree shaking #29

merged 31 commits into from
Jul 18, 2020

Conversation

IMax153
Copy link
Collaborator

@IMax153 IMax153 commented Jul 16, 2020

This PR addresses several issues with tree shaking and fixes one potential bug with the code-frame module. In addition, the PR standardizes all modules to use a common format for exports (unless the export has overloaded signatures) with updated tags indicating the category of the export.

Let me know what you think!

Summary of changes:

  • Breaking Change
    • upgrade to fp-ts@2.7.0 (@IMax153)
    • split "mega" parser instance into individual instances (@IMax153)
      • Add Functor instance
      • Add Applicative instance
      • Add Monad instance
      • Add Alt instance
      • Add Alternative instance
    • remove pipeable from Parser module (@IMax153)
  • New Feature
    • add Semigroup instance to ParseError (@IMax153)
  • Bug Fix
    • account for all common line terminators in code-frame (@IMax153)
  • Polish
    • make ParseError and ParseSuccess models readonly in ParseResult (@IMax153)
    • standardize export declarations in all modules (@IMax153)
    • add category tags to module exports (@IMax153)
  • Internal
    • make Location model readonly in code-frame (@IMax153)

@gcanti
Copy link
Owner

gcanti commented Jul 16, 2020

@IMax153 awesome, thanks!

Add Semigroup instance to ParseResult

mhhh I'm not sure we should export such an instance

p.s.

import { chain as chainEither, isRight, map as mapEither, mapLeft } from 'fp-ts/lib/Either'

AFAIK there's no need of this, import * as E from 'fp-ts/lib/Either' should be just fine for tree shaking

@IMax153
Copy link
Collaborator Author

IMax153 commented Jul 16, 2020

I'm not sure we should export such an instance

No problem! I can remove and revert the implementation of extend back to the previous iteration if you prefer, or I can leave the logic in and make the instance private. Let me know!

AFAIK there's no need of this, import * as E from 'fp-ts/lib/Either' should be just fine for tree shaking

Gotcha, I wasn't sure but I suppose since we are rewriting the import paths on build this should be a non-issue.

@IMax153
Copy link
Collaborator Author

IMax153 commented Jul 16, 2020

For now, I just internalized the Semigroup instance.

@gcanti
Copy link
Owner

gcanti commented Jul 17, 2020

About the breaking changes:

  • upgrade to fp-ts@2.7.0

is this necessary?

The following don't look like breaking changes:

  • split "mega" parser instance into individual instances (<= this is additive)
  • remove pipeable from Parser module (<= this is internal)

However this is an actual breaking change:

  • make ParseError and ParseSuccess models readonly in ParseResult

I would revert and postpone any breaking change until fp-ts v3 comes out

@IMax153
Copy link
Collaborator Author

IMax153 commented Jul 17, 2020

About the breaking changes...

Apologies - no this is not necessary. As I mentioned in the other PR, I will be much more careful about changes that alter the API in the future.

I reverted the changes to ParseError and ParseSuccess and made a TODO to make them readonly in the next major version. I also reverted the upgrade to fp-ts as it was not strictly necessary.

The following don't look like breaking changes

Hmm - I suppose these changes are backwards compatible! I updated the CHANGELOG.

To summarize the changes contained in this PR:

0.7.0

  • New Feature

  • Bug Fix

    • account for all common line terminators in code-frame (@IMax153)
  • Polish

    • standardize export declarations in all modules (@IMax153)
    • add category tags to module exports (@IMax153)
  • Internal

    • remove pipeable from Parser module (@IMax153)
    • make Location model readonly in code-frame (@IMax153)

@gcanti
Copy link
Owner

gcanti commented Jul 17, 2020

@IMax153 nice, thanks!

One last thing: since now there's no breaking changes, the next release will be tagged as 0.6.7 (when major version is zero, a bump of the minor version means a breaking change)

@IMax153
Copy link
Collaborator Author

IMax153 commented Jul 17, 2020

@gcanti Thank you very much (as always) for the teaching and guidance - it is greatly appreciated! I will plan to merge tomorrow morning.

0.6.7

  • New Feature
  • Bug Fix
    • account for all common line terminators in code-frame (@IMax153)
  • Polish
    • standardize export declarations in all modules (@IMax153)
    • add category tags to module exports (@IMax153)
  • Internal
    • remove pipeable from Parser module (@IMax153)
    • make Location model readonly in code-frame (@IMax153)

@IMax153 IMax153 merged commit aefbb00 into gcanti:master Jul 18, 2020
@IMax153 IMax153 deleted the feature/tree-shaking branch July 18, 2020 13:43
@gcanti
Copy link
Owner

gcanti commented Jul 20, 2020

@IMax153 thanks, released 👍

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