krakenjs / cross-domain-utils

Cross Domain utilities
Apache License 2.0
131 stars 18 forks source link

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

Open westeezy opened 2 years ago

westeezy commented 2 years ago
mnicpt commented 2 years ago

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

gregjopa commented 2 years ago

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 commented 2 years ago

@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:

mnicpt commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 https://github.com/krakenjs/cross-domain-utils/pull/23. Where we will move to krakenjs scopes so that we can more easily manage teams in npm as we onboard and offboard folks.