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.

Sign up for GitHub

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

Jump to bottom

build: rewrite imports from lib to es6 in es6 output #50

Merged
merged 1 commit into from
Jul 20, 2020
Merged

build: rewrite imports from lib to es6 in es6 output #50

merged 1 commit into from
Jul 20, 2020

Conversation

unekinn
Copy link
Contributor

@unekinn unekinn commented Jun 29, 2020 •
edited
Loading

Closes #46
Closes #49

Changes

  • I've copied the postbuild step and tsconfig.es6.json change from gcanti/fp-ts-rxjs@f35c926
  • io-ts-types had to be updated. The es6 folder was added in version 0.5.4.
  • In src/remote-data-io.ts, an import had to be changed to allow import-path-rewrite to rewrite it.

Testing

I tested this on a fairly large codebase, by rewriting code that used used Either and the fp-ts/es6/... imports to use RemoteData imported from @devexperts/remote-data-ts/es6/remote-data, and it worked well. webpack-bundle-analyzer indicates that only es6 parts are bundled, and as far as I can tell only the modules that are in use are included (both from fp-ts and remote-data-ts)

I did not test the remote-data-io and remote-data-t parts of the library, as I am not familiar with io-ts and monad transformers. But I inspected the generated .js and .d.ts in /dist and /es6. The output looks correct, and all the imports resolve correctly.

Let me know if you have any better ideas for testing this change!

@raveclassic
Copy link
Contributor

@larskinn Awesome! Thanks for the contribution!
@sutarmin Please take a look.

@raveclassic raveclassic requested a review from sutarmin June 29, 2020 17:32
sutarmin
sutarmin approved these changes Jul 1, 2020
View reviewed changes
Copy link

@sutarmin sutarmin left a comment

Choose a reason for hiding this comment

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

Hey, there! @larskinn, thanks a lot for your contribution! I think this PR is good to go, everything works as expected.

@unekinn
Copy link
Contributor Author

unekinn commented Jul 2, 2020

I think this PR is good to go, everything works as expected.

That's good to hear!

@sutarmin @raveclassic One of you folks will have to merge, at your convenience 😃

@sutarmin
Copy link

sutarmin commented Jul 2, 2020

Sure, I just want to find out why our CI integration is broken. I hope we can wait with the merge for just couple of days at most.

@unekinn
Copy link
Contributor Author

unekinn commented Jul 3, 2020 •
edited
Loading

Of course, no problem.

@sutarmin
Copy link

PR build passed. I don't know how to trigger branch build without additional commits, but PR build is enough.
Thanks for your contribution and patience, @larskinn, we are merging it

@sutarmin sutarmin merged commit 40a304a into devexperts:master Jul 20, 2020
@unekinn
Copy link
Contributor Author

unekinn commented Aug 10, 2020

Any chance of publishing a new release soon-ish @raveclassic ?

@raveclassic
Copy link
Contributor

@larskinn Oops, sorry, this slipped my mind. Just a moment.

@raveclassic
Copy link
Contributor

raveclassic commented Aug 11, 2020 •
edited
Loading

@larskinn Done. @devexperts/[email protected] is released. Thank you for your contribution and for your patience!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sutarmin sutarmin sutarmin approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

sequenceS and sequenceT from fp-ts Apply gives type errors Rewrite imports in es6 folder
3 participants
@unekinn @raveclassic @sutarmin