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.

Sign up for GitHub

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

Jump to bottom

Add projects to test typescript support with different compiler options #4597

Merged
merged 3 commits into from
Sep 17, 2024
Merged

Add projects to test typescript support with different compiler options #4597

merged 3 commits into from
Sep 17, 2024

Conversation

JesusTheHun
Copy link
Contributor

@JesusTheHun JesusTheHun commented Apr 17, 2024 •
edited
Loading

Following #4218, this PR adds three test projects that use ESM and CJS with npm and one that uses ESM with pnpm.

The projects are compiled with different typescript compiler options to ensure compatibility.

This PR focuses on testing that antlr4 can be imported in TS projects. This is why it does not include non-TS test projects.

@JesusTheHun JesusTheHun force-pushed the test/typescript-compiler-options branch from 613ff23 to 2d2b26c Compare April 17, 2024 14:46
ericvergnaud
ericvergnaud requested changes Apr 18, 2024
View reviewed changes
runtime/JavaScript/spec/imports/setups/node-cjs-ts/test.sh Outdated Show resolved Hide resolved
runtime/JavaScript/spec/imports/setups/node-esm-ts/test.sh Outdated Show resolved Hide resolved
runtime/JavaScript/package.json Outdated Show resolved Hide resolved
@ericvergnaud
Copy link
Contributor

Looks like the tests fail ?
See for example https://github.com/antlr/antlr4/actions/runs/8723991967/job/23933679496?pr=4597

@JesusTheHun
Copy link
Contributor Author

Looks like the tests fail ? See for example https://github.com/antlr/antlr4/actions/runs/8723991967/job/23933679496?pr=4597

I fail to run the tests locally using maven. I haven't touch to java in a while so I'm a bit rusty at identifying compilation chain issues 😅
In addition to the requested changes I've committed a fix, hopefully the one we need.

ericvergnaud
ericvergnaud requested changes Apr 18, 2024
View reviewed changes
Copy link
Contributor

@ericvergnaud ericvergnaud left a comment

Choose a reason for hiding this comment

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

Fixes the failing tests, but not sure it satisfies the requirement to test against the built package (vs the source project)

runtime/JavaScript/spec/imports/builds/node-cjs-ts/package.json
"author": "",
"license": "ISC",
"dependencies": {
"antlr4": "file:../../../.."
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't we be using a local build rather than the source folder ?

runtime/JavaScript/spec/imports/builds/node-esm-ts/package.json
"author": "",
"license": "ISC",
"dependencies": {
"antlr4": "file:../../../.."
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't we be using a local build rather than the source folder ?

@JesusTheHun
Copy link
Contributor Author

JesusTheHun commented Apr 18, 2024 •
edited
Loading

Fixes the failing tests, but not sure it satisfies the requirement to test against the built package (vs the source project)

I was hesitant to test against the build because that would require to compile every time you make a change, which is annoying during development. But I can make the change if you want. It is safer indeed.

EDIT : your build process bundle the javascript, but do not include the type definitions. So we would have to use npm pack, extract the archive, and use the directory at the package path.
That being said, what is tested really is the declaration of types definition, which only targets the package.json really. Assuming the referenced files exist. Before my latest commits they did not, and the build failed. So I say the tests served their purpose 👍

@JesusTheHun
Copy link
Contributor Author

@ericvergnaud just pinging in case you didn't see my edit above

@ericvergnaud
Copy link
Contributor

ericvergnaud commented Apr 18, 2024 •
edited
Loading

@ericvergnaud just pinging in case you didn't see my edit above

Ah I see what you mean. But how can we be sure that your test samples are using the dist folder ?

@JesusTheHun
Copy link
Contributor Author

JesusTheHun commented Apr 18, 2024 •
edited
Loading

Ah I see what you mean. But how can we be sure that your test samples are using the dist folder ?

The test uses the package.json. The package.json references the dist folder for the javascript, and the src for the type definitions.

EDIT : to clarify, npm pack is the command that create the archive that is published. It copies the package.json as-is, so our tests have the exact same conditions.

@JesusTheHun
Copy link
Contributor Author

JesusTheHun commented May 1, 2024 •
edited
Loading

@ericvergnaud after migrating the project using this lib to pnpm, I noticed the types stopped to work altogether. After looking around I noticed that the imports inside the definition files did not include the file extension, which is required for modern node usage.

EDIT : for anyone following this topic, I've published a package that reflect the changes on this branch : https://www.npmjs.com/package/@cbjsdev/antlr4 . Install the latest version 4.13.2.

@ericvergnaud
Copy link
Contributor

Hi, sorry for the delay.
I suspect that the reason you need the .js extensions is because the tests are running against the source code rather than the built package.
Node does not support TypeScript, and should be using the web packed module.
I suggest you revert commit 40bb02c, which makes this PR way too broad.

@JesusTheHun
Copy link
Contributor Author

JesusTheHun commented Jul 25, 2024 •
edited
Loading

@ericvergnaud The need for the extension comes from the node16 TypeScript target.
TypeScript uses the d.ts files, so it's unrelated to the packaged source files.

@ericvergnaud
Copy link
Contributor

ericvergnaud commented Jul 25, 2024 •
edited
Loading

@JesusTheHun sorry but I don't follow:

  • I've been using these .ts files "as is" for years now without any problem
  • Typescript uses the .ts files, and doesn't need the .js extension
  • node16 is unaware of these .ts files, and should be using the webpacked module

What am I missing ?

It seems the issue appears when introducing pnpm ? That's a separate topic altogether.
Can we restrict this PR back to its primary intent ?

(FWIW we are preparing a release, I'd like to get some of this in)

@JesusTheHun
Copy link
Contributor Author

@ericvergnaud When I mention node16 I'm talking about the TS compiler option "target". Not the node runtime version.

pnpm is the most strict package manager. It enforces standards and prevent bad usage instead of working around them. This may be the reason why only pnpm issues errors.

The intent of the PR is to make sure projects can import Antlr without issue, right ?
If so, the commit is required, otherwise pnpm users won't be able to import Antlr.

Also, I'm on holidays without my machine. So I wont be able to revert the commit myself before August 5th.

That being said, I'm curious about the reason for rejecting the commit. Ok it makes a lot of file changes, but they are trivial changes, it's not like they each require a review.

@ericvergnaud
Copy link
Contributor

The reason is that I'm in favor of small steps.
We currently don't support pnpm and I don't need that additional responsibility (similarly we only support webpack).
So let's keep this PR as small as possible and ensure it covers the currently supported environments:

  • node >= 16
  • typescript >= 4.8
  • webpacked code from the dist folder

@JesusTheHun
Copy link
Contributor Author

JesusTheHun commented Jul 26, 2024 •
edited
Loading

I understand the need for small steps.

pnpm falls into the support for node >= 16, since it's officially distributed by Node.js via corepack.

I understand the commit touches a lot of files but the changes are tiny and identical in each file.
I don't see any drawback.

@JesusTheHun
Copy link
Contributor Author

JesusTheHun commented Aug 1, 2024 •
edited
Loading

@ericvergnaud I'll be back in a few days, can you hold the release until then ?
I assume you also want me to delete the test project for pnpm ?

@ericvergnaud
Copy link
Contributor

@parrt not sure what is your ETA for the release ?

@ericvergnaud
Copy link
Contributor

@ericvergnaud I'll be back in a few days, can you hold the release until then ? I assume you also want me to delete the test project for pnpm ?

Yes please rollback pnpm related changes

@parrt
Copy link
Member

parrt commented Aug 2, 2024 via email

@ericvergnaud
Copy link
Contributor

@JesusTheHun hi there, it's been a while... Any progress ?

@JesusTheHun JesusTheHun force-pushed the test/typescript-compiler-options branch from f7d2479 to 6b49b52 Compare September 16, 2024 11:03
@ericvergnaud
Copy link
Contributor

@JesusTheHun Thanks, this is looking good. The TS tests fail though. Could you take a look ?

@JesusTheHun
Copy link
Contributor Author

@JesusTheHun Thanks, this is looking good. The TS tests fail though. Could you take a look ?

This looks unrelated to this PR, as a previous action failed the same way. See https://github.com/antlr/antlr4/actions/runs/10863756846/job/30148277515

@ericvergnaud
Copy link
Contributor

@JesusTheHun Can you rebase ?

@JesusTheHun
test: create ts projects using esm and cjs
5f6067c 
Signed-off-by: Jonathan MASSUCHETTI <jonathan.massuchetti@dappit.fr>
@JesusTheHun
misc: rename "projects" to "builds
9fbb677 
Signed-off-by: Jonathan MASSUCHETTI <jonathan.massuchetti@dappit.fr>
@JesusTheHun
build(typescript): reference the new definition file in the package.j…
f131fa9 
…son "types" fields

Signed-off-by: Jonathan MASSUCHETTI <jonathan.massuchetti@dappit.fr>
@JesusTheHun JesusTheHun force-pushed the test/typescript-compiler-options branch from 6b49b52 to f131fa9 Compare September 17, 2024 09:06
@JesusTheHun
Copy link
Contributor Author

@ericvergnaud all clear ☝️

@ericvergnaud
Copy link
Contributor

@parrt blessed
@JesusTheHun Many thanks for this !!!

@parrt parrt merged commit 2a7904a into antlr:dev Sep 17, 2024
42 checks passed
@JesusTheHun JesusTheHun mentioned this pull request Oct 25, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ericvergnaud ericvergnaud ericvergnaud requested changes

Assignees
No one assigned
Labels
target:typescript
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@JesusTheHun @ericvergnaud @parrt