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

Produce esm modules during prepublish scripts #256

Merged
merged 4 commits into from
Jun 28, 2021

Conversation

evansiroky
Copy link
Contributor

@evansiroky evansiroky commented Jun 9, 2021

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:

  • a new script build:esm is added
  • linting and jest ignores these newly created esm directories in each package
  • The babel config file is changed to facilitate the production of the esm files.
  • Each package now has a module entry point
  • All imports of packages using a path to some \lib\ directory are changed to import from the root export. Some packages were modified to better export variables to facilitate this conversion.

@evansiroky evansiroky added the WIP Work in progress label Jun 9, 2021
@evansiroky evansiroky self-assigned this Jun 9, 2021
@binh-dam-ibigroup
Copy link
Collaborator

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?

Copy link
Collaborator

@binh-dam-ibigroup binh-dam-ibigroup left a 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";
Copy link
Collaborator

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?

Copy link
Contributor Author

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.

@binh-dam-ibigroup binh-dam-ibigroup removed their assignment Jun 21, 2021
Copy link
Member

@landonreed landonreed left a 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?

@evansiroky
Copy link
Contributor Author

evansiroky commented Jun 22, 2021

@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 esm folder and package.json for one of these packages and add/replace the folder/file into the respective node_modules directory in trimet-mod-otp and then put in a log statement, you'll be able to confirm that webpack loads the esm file. The benefits of this are that webpack can do tree shaking because these files in the esm folders use the import/export syntax (see the first paragraph of the webpack doc page about tree shaking).

@evansiroky evansiroky removed their assignment Jun 22, 2021
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.

PERFORMANCE: import statements, ../lib/... paths and tree-shaking
6 participants