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

BREAKING CHANGE: Move dist/module to lib. Export Flow types #21

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

westeezy
Copy link
Contributor

  • Remove module folder
  • Output transpiled code and flow defs to lib/

@westeezy westeezy requested a review from a team as a code owner February 15, 2022 00:58
@mnicpt
Copy link
Contributor

mnicpt commented Feb 15, 2022

@westeezy Why are we wanting to do this? Can you explain more the reason behind this change?

@gregjopa
Copy link
Contributor

Great work @westeezy!

I agree we @mnicpt that we should document the details in the PR description to describe the benefits with this approach.

I think the big win here is eliminating the need to import from /src. Ex:

Before:

import { getUserAgent } from 'cross-domain-utils/src';

https://github.com/krakenjs/post-robot/blob/dff05abe99bcc2018afd0eb6fbbda7e4811ac295/src/lib/compat.js#L3

After:

import { getUserAgent } from 'cross-domain-utils';

With this PR the above import statement still gets the Flow types as well as supports the existing tree-shaking strategy.

This change is a pre-req to converting this repo to TypeScript while still supporting Flow types for other repos that are still using Flow.

@westeezy
Copy link
Contributor Author

westeezy commented Feb 15, 2022

@mnicpt we are currently directly importing flow files when we grab files from src. This means any project importing those files must also be setup with flow. This isn't a good practice and especially as we move to typescript we need to export esm modules without typings attached. To continue to provide type support we will output ts and flow definition files so you should have no interruption in your workflow. This PR keeps both src and lib in the tarball so you will notice nothing new and later we will fully remove/deprecate src. This is a breaking change for anyone using dist/module which we do not internally. We could consider keeping the esm files in dist/module as well which I am not opposed to as lib was chosen simply because of it being a convention that is a bit more ergonomic if we can't use import maps (needs testing).

@gregjopa for the direct import we will utilize package export maps. I have to test that feature more in webpack@4 so for now we are keeping the src folder for that reason and a later change will add the import map to enable import { getUserAgent } from 'cross-domain-utils' vs import { getUserAgent } from 'cross-domain-utils/lib'

edit: package exports not supported in webpack v4. womp womp. will need to import from lib or another folder

TODO:

  • update commit message with reason why for history

@mnicpt
Copy link
Contributor

mnicpt commented Feb 15, 2022

Thanks @gregjopa @westeezy ! This is great!

later we will fully remove/deprecate src

So where we have this import { getUserAgent } from 'cross-domain-utils/src'; in current repos, we will be able to keep it in the future? Seems like we'll have to update all of these.

@westeezy
Copy link
Contributor Author

Yeah @mnicpt the plan is to allow importing from either src or lib until everything is moved over to lib. So we are going to take it slow in order to ensure we don't have any regressions. I will be providing a list of update locations as we go and we will likely keep src as an option for a bit until we are 100% confident we can remove.

@westeezy
Copy link
Contributor Author

Also I am thinking of adding a lint rule in the future to ensure @krakenjs packages are imported from lib and not src but that won't be until all the repos are in the right spot. I am not 100% sure we will need the lint rule but it is an option to add additional safety to the migration and would be quick to do.

See #23. Where we will move to krakenjs scopes so that we can more easily manage teams in npm as we onboard and offboard folks.

Moving esm files to `lib/` folder gives us a path to stop importing from
`src/` when importing files for treeshaking. We can migrate to importing
from `lib/` and still have flow definitions at the ready

Tasks Completed:
- Remove module folder
- Output transpiled code and flow defs to `lib/`
Copy link
Contributor

@mnicpt mnicpt left a comment

Choose a reason for hiding this comment

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

Great job, @westeezy !

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.

3 participants