search

date-fns / date-fns-upgrade

A tool for upgrading date-fns versions
8 stars 8 forks source link

Fix broken tree shaking for legacy parse #15

Open marcodejongh opened 3 years ago

marcodejongh commented 3 years ago

Fixes #14

marcodejongh commented 3 years ago

@dkozickis @kossnocorp @silvenon Could you do us a huge favour and review and merge this? It's a relatively low-risk change that has a big impact, without this change we're not able to move forward with our date-fns upgrade, unless we remove date-fns/upgrade as a dependnecy.

marcodejongh commented 3 years ago

Needed to fix tsconfig module resolution, wonder if that is why the old style import was used to begin with. We're now using this version in prod published as date-fns-upgrade-treeshaking@1.0.8, so should be good to go now

silvenon commented 3 years ago

Could you help reviewers understand what the problem is and how these changes fixes it? I think it would greatly increase the chances of merging.

marcodejongh commented 3 years ago 
import { isDate } from 'date-fns'

Imports the whole module and doesn't get tree-shaken by Webpack. By changing this to:

import isDate from 'date-fns/isDate'

There is no more need to tree-shaken because we're now importing from a single dedicated file.

The changes to the config were necessary to facilitate those changes as TS is a bit fickle in its module resolution logic. date-fns exports isDate on the module.exports it loses its "default" key, by enabling esModuleInterop, TS protects against this scenario, resulting in the code working.

silvenon commented 3 years ago

Adding an ES build of this library seems like a more natural solution for this problem, that way people won't end up with two copies of isDate (date-fns/isDate and date-fns/esm/isDate).

marcodejongh commented 3 years ago

@silvenon Not sure I understand? date-fns currently doesn't have an ESM build: https://unpkg.com/browse/date-fns@2.21.1/isDate/ The recommended method of importing date-fns: https://date-fns.org/docs/Getting-Started#submodules

You're right that if:

import { isDate } from 'date-fns'

Was ESM, we would need to do this change, but the fact that date-fns didn't add an ESM build up until now, leads me to believe this is an intentional decision. Arguably, we'd still want to import by submodules as this is still more reliable than tree shaking.

silvenon commented 3 years ago

Only now I realized I don't have merging rights. 🤦‍♂️ 🤦‍♂️ 🤦‍♂️ @dkozickis

marcodejongh commented 3 years ago

Ah it does have ESM, missed that...

I just noticed that date-fns/upgrade didn't tree shake properly when bundle analysing, I'm not quite sure why it doesn't tree shake https://unpkg.com/date-fns@2.21.1/esm/index.js looks fine, but Webpack can be a bit fickle sometimes, it's better to just use the submodule import instead.

silvenon commented 3 years ago

Because @date-fns/upgrade only has a CJS build, so that's where the tree-shaking potential stops. 😕 You'll need to edit Makefile to produce two builds. To create an ESM build you can probably just pass flags to tsc, and make sure to add module field to package.json. Let me know if you need additional help.

marcodejongh commented 3 years ago

Adding ESM sounds like a good follow-up task on this project, but has significantly bigger scope and as a result a higher risk profile. This PR is significantly less risky and fixes the more immediate problem of pulling in all of date-fns when trying to use this upgrade module.