-
Notifications
You must be signed in to change notification settings - Fork 36
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
Produce esm modules during prepublish scripts #256
Conversation
With the changes in this PR, do we also want to deprecate the use of destructured declarations of module exports such as in this example, whether they occur in otp-ui or otp-react-redux? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Two thoughts:
- It doesn't look like any changes are breaking as the now-preferred alternative has always been available, but others can override me on that.
- We could use destructuring declarations such as these for methods that we use most often in a file, but I don't feel strongly about that.
calculatePhysicalActivity | ||
} from "@opentripplanner/core-utils/lib/itinerary"; | ||
import { mergeMessages } from "@opentripplanner/core-utils/lib/messages"; | ||
import { formatTime } from "@opentripplanner/core-utils/lib/time"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This import will need to be updated in OTP-RR, so should this be noted as breaking?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe this is breaking since the lib
folder is still created during the prepublish script. Therefore, these imports should still work. These imports of course wouldn't use the files from the esm folder, but things should just work when importing from package root.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@evansiroky, could you provide some more info on how to leverage this approach and what it offers in the project README?
I don't think this is worthy of an entry within the README. In various other 3rd-party packages that produce various kinds of files, they don't bother mentioning it (see auth0-react, connected-react-router and yup). This module thing "just works" when using a bundler that is aware of modules which should be webpack 2+. If you copy the generated |
This fixes #65 by adding a separate build script that will produce esm modules that can be imported by other packages using the
module
entry point in the package.json file.Specifically, the changes include:
build:esm
is addedmodule
entry point\lib\
directory are changed to import from the root export. Some packages were modified to better export variables to facilitate this conversion.