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

Make cucumber-expressions compatible with both cjs and esm #1743

Merged
merged 20 commits into from
Sep 17, 2021

Conversation

aurelien-reeves
Copy link
Contributor

This is another approach to gently make the packages compatible with ESM.

Refs. #1709

The idea is pretty close to PR#1709. Main differences are:

  • no code, only configuration
  • make packages officially supporting CJS and ESM
  • iterative approach: we can move-on package per package, one PR at a time

The global idea is:

  • from our point of view, packages are now of the type module (ESM)
  • tests are executed with the ts-node experimental ESM loader - which will hopefully not be experimental anymore some day
  • we build two versions of the packages: one CJS, and one ESM
  • we have to add those annoying .js extensions anyway 🙄
  • the iterative approach - working one package at a time - allows us to pay more attention on the exports we are proposing to make sure they remain retro-compatible
  • migrating dependent packages should be easier, and not mandatory as part of a single PR. As long as they still are using CJS, they will automatically use the CJS version of the packages. But when migrating to ESM, they will automatically switch to the ESM version

So, JavaScript folks (@aslakhellesoy @charlierudolph @davidjgoss, and actually anyone) what do you think?

I have already used that technique for reindent-template-literals, so if you want to take a look on how it works, you can take a look on the tiny package here: https://gitlab.com/kao98/reindent-template-literals. It has some acceptance tests which illustrate the usage of such package: https://gitlab.com/kao98/reindent-template-literals/-/tree/main/tests

@aurelien-reeves
Copy link
Contributor Author

Yes, the build still have to be fixed. I already have some ideas for that, this is not an issue (yet 😅)

@aurelien-reeves
Copy link
Contributor Author

It should be ready now.

cucumber-expressions is now a module, and build into esm and cjs.
There are now 3 typescript builds: one "legacy", one "esm" and one "cjs".

The legacy one will be removed once all the packages in the monorepo will be converted.

We will need the esm and cjs ones as long as we will support cjs.

It looks like a lot of files to review. But actually, all the .ts ones just have changes into their import statement: the addition of the ".js" extension when importing module using relative paths. So you can review those very quickly.

There is a new template project javascript-esm which is dedicated for those kind of packages.

Also you may notice that there is that dist/cjs/package.json file: this is what makes possible to have both esm and cjs compatible builds inside the same npm package. There are other ways to do so, but I pick that one because it is the more transparent one for the user: he just has nothing to do whatever he is using cjs or esm.

Copy link
Contributor

@davidjgoss davidjgoss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable to me.

One query: this whole topic got kicked off because some packages that we depend on are starting to publish as ESM only, which causes us an issue because you can't require ESM modules. So with that in mind, how does the CommonJS code that we generate in the "cjs" version here handle those cases?

"extends": "./tsconfig.build.json",
"compilerOptions": {
"lib": [
"es6"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW I don't think we need to go as far down as ES5 or ES6 - our minimum support is Node 12 so it can be ES2019.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I'll give it a try

@aurelien-reeves
Copy link
Contributor Author

Looks reasonable to me.

One query: this whole topic got kicked off because some packages that we depend on are starting to publish as ESM only, which causes us an issue because you can't require ESM modules. So with that in mind, how does the CommonJS code that we generate in the "cjs" version here handle those cases?

Indeed, we'll have the same issue.
Not there yet. The idea is to move on gently with that topic to reduce the difficulty once we get there.

We could imagine, for example, migrating all @cucumber/packages one by one using the proposed cjs/esm technique. That way we may be able to convert cucumber-js relatively easily. Then once cucumber-js has been converted, we can update those dependencies by removing the support for cjs just for the impacted packages.

I don't say we will be able to do it that way. But this is at least a first step into moving to esm.

@aurelien-reeves
Copy link
Contributor Author

@aslakhellesoy no objection into merging this PR?

@aslakhellesoy
Copy link
Contributor

The cjs packages we build will fail at runtime if they depend on esm-only modules.

There is no point in releasing a package that doesn’t work, so we should test that it works before we release it.

Can we add a test-cjs npm script to this PR before merging? It would run the mocha tests against the compiled cjs code.

@aurelien-reeves
Copy link
Contributor Author

The cjs packages we build will fail at runtime if they depend on esm-only modules.

cucumber-expressions does not depend on esm-only modules.
I was talking about other packages - cucumber-react mainly. But as I explained, the idea is to move-on step by step to reduce the difficulty when we will hit that point.

There is no point in releasing a package that doesn’t work, so we should test that it works before we release it.

Can we add a test-cjs npm script to this PR before merging? It would run the mocha tests against the compiled cjs code.

We could, but there is no point to do so. The build would have failed in such case.

And fake-cucumber validates the cjs build for cucumber-expression already. fake-cucumber is still using the cjs version of cucumber-expression. And fake-cucumber tests and build are still green.

@aurelien-reeves
Copy link
Contributor Author

I am actually facing some difficulties: while I was trying to move messages into a hybrid package, I have not been able to find a way to read a file - or require a json file - using a relative path.

With ESM, there is no __dirname but import.meta.url instead. It is working fine with modules. But when it appears in a source code executed as cjs package, Node throws a SyntaxError at compile time that I have not been able to avoid.

Any idea how we could going through this?

@aslakhellesoy aslakhellesoy added library: cucumber-expressions 🔧 build Related to build / release process labels Sep 15, 2021
@aurelien-reeves
Copy link
Contributor Author

With @aslakhellesoy we have identified a workaround to avoid the issue I mentioned earlier

@aurelien-reeves aurelien-reeves mentioned this pull request Sep 16, 2021
@aurelien-reeves
Copy link
Contributor Author

@aslakhellesoy agree to merge?

@aslakhellesoy aslakhellesoy merged commit 780803c into main Sep 17, 2021
@aslakhellesoy aslakhellesoy deleted the make-cucumber-expressions-esm-compatible branch September 17, 2021 15:01
@aslakhellesoy
Copy link
Contributor

Awesome!!!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔧 build Related to build / release process
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants