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

fix(#9): Module error when using Javascript #12

Merged
merged 4 commits into from
Jan 17, 2025

Conversation

m-t-a97
Copy link
Contributor

@m-t-a97 m-t-a97 commented Jan 16, 2025

What changes are being made and why?

This PR aims to address CJS and ESM modules compatibility as the current version of the project is broken and no longer usable. The current version of the project along with another PR attempted to introduce ESM and CJS compatibility but introduced several issues, including increased complexity, potential for inconsistencies, and breaking changes for existing users. To resolve these issues comprehensively and provide a future-proof solution, significant improvements and changes to this project were made:

  • Refactored Build System to Use Rollup with TypeScript
  • Replaced the project's JavaScript setup with TypeScript to provide stronger type safety, making the codebase easier to maintain and less error-prone.
  • Integrated Rollup plugins such as @rollup/plugin-typescript to handle .ts files and much more.
  • Configured Rollup to produce both CommonJS (CJS) and ECMAScript Module (ESM) bundles as well as *.d.ts files providing full type safety for users running TypeScript too.
  • Ensured compatibility with the package.json exports field to provide separate entry points for CJS and ESM users.
  • Resolved Issues with Module Format Handling.
    • Problem: Another PR introduced .cjs and .es6.js file extensions, adding complexity and maintenance overhead.
    • Solution: use a bundler like Rollup to generate appropriate bundles without needing to manually rename files.
  • Ensured users can seamlessly use the library as either CJS or ESM based on their environment along with support for TypeScript.
  • Improved Package Structure
  • Updated package.json:
    • Added exports field for clear module resolution.
    • Set appropriate main, module, and types fields pointing to the bundled files.
    • Removed redundant fields and ensured alignment with modern Node.js module resolution practices.
    • Added dist to the files field to control what gets published to npm.
  • Improved Automated Testing:
    • Replaced Jest in favour of Vitest. A faster alternative to Jest but compatible with Jest's API.
  • Prevented Breaking Changes:
    • Retained the existing API structure where possible to avoid breaking existing user code.
    • Ensured the default export (Kestra function) remains consistent, allowing users to upgrade seamlessly.
  • Added prettier for code formatting
  • Added eslint for code linting
  • Added .nvmrc setting project node version to current LTS (at the time of writing it is lts/jod

How the changes have been QAed?

These are the steps required to reproduce the tests:

  • Create any JS based project. It can literally be anything such as a CJS, ESM or TypeScript based project as this PR provides full support for all of these now.
  • Feel free to write any code e.g. an express.js server or a simple node.js script etc along with any other third party libraries you'd like to use.
  • Make sure to have this repo cloned and switch the branch to this PR.
  • Then follow the steps outlined in the README.md under the javascript folder as it outlines how you can test this library.

The general gist of testing all this is as follows:

  • Symlink this library to your global node_modules
  • Navigate to the external project, npm install the @kestra-io/libs package as a dependency
  • npm link the kestra package to the external project. Now your project will be picking up the source code from the symlinked @kestra-io/libs project.
  • If you already have for example a basic JavaScript project with a package.json and index.js file, you can simply use CJS and it will import the CJS bundle from our Kestra library. I'd recommend creating a simple express.js server using the following tutorial: https://expressjs.com/en/starter/hello-world.html. This will give you an insight into how this library works in a real use case.
  • If you want to test the code using ESM, simply change to "type": "module" in package.json and your project will be changed to ESM now and you are not required to change the file extension at all, everything will work seamlessly. But you will need to change the syntax to use ESM utilising import and export etc instead of the require syntax.
  • For use with TypeScript, Create a TS project as usual with tsconfig etc, symlink the same way using npm as a package manager and you should get full type safety due to the library automatically providing *.d.ts files.

And there you have it, full support for CJS, ESM and TypeScript with this library without any breaking changes 🎉.

P.S. I would like some feedback and more people to test this so that everything works as mentioned so feel free to post in the comments if it all works for you.


Closes issue #9

@m-t-a97 m-t-a97 changed the title fix(#9): Fixed overall issue with the JavaScript library fix(#9): Module error when using Javascript Jan 16, 2025
@m-t-a97 m-t-a97 changed the title fix(#9): Module error when using Javascript [WIP] - fix(#9): Module error when using Javascript Jan 16, 2025
fix: Removed my username from author
@m-t-a97 m-t-a97 changed the title [WIP] - fix(#9): Module error when using Javascript fix(#9): Module error when using Javascript Jan 16, 2025
@elevatebart
Copy link
Contributor

Wow !!! Thank you @m-t-a97 . Great work!

I need to go through the code one more time but this looks great to go.

I'll merge ASAP.

@m-t-a97
Copy link
Contributor Author

m-t-a97 commented Jan 16, 2025

Thank you very much @elevatebart, really appreciate it 👍. Can't wait to see it merged in!

@m-t-a97
Copy link
Contributor Author

m-t-a97 commented Jan 16, 2025

@elevatebart can you hold out for a moment actually, I need to make a quick update to the code to add some type annotations for JavaScript as the Kestra function methods don't have any intellisense for some reason. It works fine with .ts files as they come with *.d.ts files already. So let me quickly add the type annotations to the code so that for .js files, they also have types showing up in the intellisense. I'll also do some testing to make sure it works and provide an update after this comment on how to test it again.

@m-t-a97 m-t-a97 changed the title fix(#9): Module error when using Javascript [WIP] - fix(#9): Module error when using Javascript Jan 16, 2025
@elevatebart elevatebart added the kind/do-not-merge Don't merge label Jan 16, 2025
@elevatebart
Copy link
Contributor

I have only one worry about backwards compatibility.
I think you now 'export default' even in cjs instead of exporting natively.

I need to check.

If you do this before this PR you get the following output.

const Kestra = require('@kestra-io/libs')

console.log(Kestra.toString())
// output: function Kestra() {}

with your change you get this because now we export default.

const Kestra = require('@kestra-io/libs')

console.log(Kestra.toString())
// output: [Object object]

I'll get back to you ASAP

@elevatebart
Copy link
Contributor

OK I lied, looks good to me

@m-t-a97
Copy link
Contributor Author

m-t-a97 commented Jan 16, 2025

No worries. Thanks for checking it out 👍. I'm still looking into how to get intellisense and types to show up for CJS. I'll get back to you with another update once I've figured it out.

@m-t-a97
Copy link
Contributor Author

m-t-a97 commented Jan 17, 2025

Ok so I figured out what the issue was yesterday. Basically when bundling the code with rollup and you use export default, the generated output files doesn't have the correct setup for default exports.

For esm it does this:

kestra.mjs:

// Minified output
export { t as default };

kestra.d.mts:

import { KestraFunction } from '../types/kestra.mjs';

declare const Kestra: KestraFunction;

export { Kestra as default };

For cjs:

index.mjs:

import r from './kestra/kestra.mjs';

export { r as default };

index.d.ts:

import Kestra from './kestra/kestra.js';

export { Kestra as default };

The problem with this is that it adds a .default prop to the Kestra function in order to access its methods. Apparently this is like the default behaviour of how esm works. This now introduces a breaking change so I'm trying to look into how to generate the code and type definitions so that it respects the default export and doesn't add the .default prop to the exported Kestra function. So the correct output to generate would be:

index.d.mts:

import Kestra from './kestra/kestra.mjs';

export default Kestra;

index.d.ts:

import Kestra from './kestra/kestra.js';

export = Kestra;

You can actually see an example of how rollup works here in their repl regarding default exports and what it generates: https://rollupjs.org/repl

@elevatebart
Copy link
Contributor

Do you want me to check it out?
Or do you feel like you will get through it yourself?

@m-t-a97
Copy link
Contributor Author

m-t-a97 commented Jan 17, 2025

I think I'm quite close to getting this working. I'll push my code later today so you can check it out, hopefully by then I've got it all working.

@m-t-a97 m-t-a97 changed the title [WIP] - fix(#9): Module error when using Javascript fix(#9): Module error when using Javascript Jan 17, 2025
@m-t-a97
Copy link
Contributor Author

m-t-a97 commented Jan 17, 2025

@elevatebart, I managed to implement it, it should all be working now. I'm pretty happy with the result. Please check it out and test it out too and let me know if you have any feedback 👍.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants