Closed jordimarimon closed 6 months ago
@jordimarimon thanks for this detailed input (and sorry for letting this slide for some time..)!
There are some relatively independent points to address:
There are imho two ways to proceed: a) Implement 1+2 and delay 3 for some time, to keep compatibility with older angular versions for some time. b) Directly implement 3 (skipping parallel/fallback versions) - this is less effort, but reduces compatiblity.
I'm not totally sure if a) or b) would be the smart move - any input?
Reading https://angular.io/guide/releases#deprecation-practices I'd rather go for a) - at least if it does not induce too much overhead.. (inputs still welcome!)
commented on your commit with some ideas, @daniel-sc -- thanks for your work!
This is a mess - anyone an expert with "jest commonjs + esm dynamic imports"? See https://stackoverflow.com/questions/77962982/testing-commonjs-with-dynamic-imports-of-esm-with-ts-jest Any help would be appreciated.. :)
Hi Daniel!
Sorry for not answering before. When I saw the answer, it was already too late and a PR had already started.
I have made a fork of the project and the following commit to fix it:
I have been able to successfully execute both the build and the test scripts.
This a fix I made with just a few minutes, probably you will want to organize things better. Also feel free to remove the any
type I have added in src/builder.spec.ts
to a better one. I just was too lazy to define a correct type here. I can define the type for you If you don't find a good one.
These are the changes I have made to make it work:
Use ts-jest/presets/default-esm
preset for ESM compatibility -> more info here
Add --experimental-vm-modules
when executing jest
-> more info here
Because now all *.spec.ts
files are ESM and not CommonJS, we don't have access to global variables like jest
or __dirname
. I have made the changes accordingly . For the __dirname
I have used the following recommendations that you can find here based also in the versions you're testing in the github workflows. I don't know if you would like to remove Node 14
from the github workflow as it is not mantained anymore based on the Node releases. Maybe I'm wrong here.
Finally I have added another tsconfig.json
for the building process... The reason why we have now two tsconfig files is because for building we want CommonJS but for testing we want ESM. I don't like having multiple tsconfig files. In reality we are not forced to add another tsconfig file If we provide the tsconfig options for testing directly throught the jest.config.ts
file. I didn't do that because if we do it, probably we will have to remove the "preset" and provide all the options that the preset sets (the options are documented here and also can be found here and here):
import {JestConfigWithTsJest} from 'ts-jest';
// Sync object
const config: JestConfigWithTsJest = {
preset: 'ts-jest/presets/default-esm', // -> This will need to be removed
testEnvironment: 'node',
verbose: false,
maxWorkers: 100, // -> maybe we don't need this large value anymore?
maxConcurrency: 1,
testTimeout: 30_000,
testMatch: undefined,
testRegex: '.*\.spec\.ts$',
collectCoverageFrom: ['src/**/*.ts'], // exclude coverage from schematic as it only collects from js (instead of ts)..
coveragePathIgnorePatterns: ['src/rmSafe.ts'],
// HERE PROVIDE THE TSCONFIG OPTIONS SO MODULE IS NOT COMMONJS
transform: {
'^.+\\.tsx?$': [
'ts-jest',
{
useESM: true,
tsconfig: {
// Provide the same that right now is in `tsconfig.json`
// https://kulshekhar.github.io/ts-jest/docs/getting-started/options#options
},
},
],
},
};
export default config;
Also If you would like to disable Node warnings (due to the use of experimental features) when executing tests, we can change (probably won't work in Windows as I think it has another way of setting environment variables):
NODE_OPTIONS="$NODE_OPTIONS --experimental-vm-modules" jest
with:
NODE_OPTIONS="$NODE_OPTIONS --experimental-vm-modules" NODE_NO_WARNINGS=1 jest
@jordimarimon thanks very much for picking this up! There is only one thing, I feel a little unsure about: How much risk do we have when diverging the module system in test and productive code? (And all this only for importing the angular version..) The alternative could be to make a major release and drop support for v16 (and before..)?
@daniel-sc
Like you said in your comment, this is a mess due to the fact that we have to build for CommonJS but test with ESM...
This shouldn't be the case. I would only support Node 18 and Node 20. I wouldn't support older versions of Node or Angular versions that are no longer mantained.
This would make easier to move to ESM entirely and stop using CommonJS as I think it's time to move on.
The changes would be the following:
"type": "module"
in the the package.json
. That way all JS files are treated as ESM by default by Node.module
and moduleResolution
in the tsconfig.json
file to Node16
or NodeNext
(I prefer NodeNext
). This will make useless the necessity of havin two tsconfig files.tsconfig.json
would require rewritting all imports to include the file extension (.js
). This is how modern Node works. I think it's a good change and makes it easier to work with other tools in the ecosystem. Here you can see how it looks like. isolatedModules: true
-> better compatibility with other bundlers that don't use the TypeScript compiler (like esbuild or SWC)useDefineForClassFields: true
-> to make sure the TypeScript code is valid JS code. This would mean stop using decorators as parameters as of right now.verbatimModuleSyntax: true
-> I do like this because it gives more information to bundlers that don't use the TypeScrupt compiler. But it's only useful if you use another bundler for transpiling TS and not the TypeScript compiler.With all theses changes we would have a modern project with TypeScript and ESM and we could even stop using Jest and all the hacks that make it work with TypeScript + ESM and just use Vitest (which it's 100% compatible with Jest). You wouldn't need presets or transforms. I have already tested Vitest with this project and it just works out of the box and you have support for coverage as well.
The only problem that right now makes it impossible to migrate to TypeScript and ESM is this line of code in the Angular CLI repo:
Because AngularCLI is still using require()
we can't publish this project with ESM because require()
can't be used to load an ESM module. It would be nice if the Angular CLI stopped using require()
and used import()
instead.
@daniel-sc
The above comment is more a suggestion for the future, not for right now as it's not possible to stop supporting CommonJS sadly...
There is only one thing, I feel a little unsure about: How much risk do we have when diverging the module system in test and productive code? (And all this only for importing the angular version..) The alternative could be to make a major release and drop support for v16 (and before..)?
Yes, you're right. Probably we would need a major version just to "notify" people.
I wil open an issue in the Angular CLI asking what it would take to stop using code that makes it not possible for third party builders to distribute ESM modules.
I want to know if it's possible for the Angular CLI to support ESM third party builders or maybe I'm missing something.
For example if the Angular CLI loads builders dynamically using require()
, it could check first if the builder is ESM and in case of being ESM use a dynamic import, otherwise use require()
.
I thinks this is the same that it's being done here although I'm not entirely sure.
I have to do a little bit of research and also see the source code of the Angular CLI to understand better how everything works right now. Everything I'm saying right now comes from my ignorance and my lack of understanding of how the Angular CLI works internally.
@jordimarimon there is already an open issue: angular/angular-cli/issues/22786
I might have found a cleaner way: import the schema.json of the angular-devkit i18n-builder and check for the attribute existence there. This way, we don't need to import the angular version and problems with CommonJS/ESM interop go away.. (of course, when/if angular allows for ESM schematics/builders we should migrate eventually) - see #104
It seems the other approach works well - can some of you please check out the beta version 2.11.0-1 and report if anything is broken?
This is resolved with v2.11.0
Angular CLI v17 deprected in the
@angular-devkit/build-angular:extract-i18n
builder thebrowserTarget
in favor of the newbuildTarget
option.If one executes the
ng extract-i18n
command using theng-extract-i18n-merge:ng-extract-i18n-merge
builder with Angular CLI version 17, they will get the following warning:If one changes in the
angular.json
in theextract-i18n
architect the optionbrowserTarget
tobuildTarget
, they will get the following error:I have never written a custom builder for Angular but I suppose that the warning comes because of this line:
https://github.com/daniel-sc/ng-extract-i18n-merge/blob/master/src/builder.ts#L110
And the error comes because of the
schema.json
:https://github.com/daniel-sc/ng-extract-i18n-merge/blob/master/src/schema.json#L113-L117
One can still use the
ng-extract-i18n-merge:ng-extract-i18n-merge
builder with Angular CLI v17 as the option has only been deprecated, not removed.It's only to give a heads up, for when Angular removes it completely.
Maybe in the future one cool thing would be to have an schematic for this builder that could be executed with
ng update
and it would internally call the update schematic of@angular-devkit/build-angular:extract-i18n
to make sure that all users have the builder options updated. But this would required a lot of work. I would be open to help though.