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(angular): fix Coveo headless error on application run #193

Merged
merged 2 commits into from
May 3, 2021
Merged

Conversation

y-lakhdar
Copy link
Contributor

@y-lakhdar y-lakhdar commented May 2, 2021

https://coveord.atlassian.net/browse/CDX-280

Proposed changes

I updated the angular tsconfig.json file to include the allowSyntheticDefaultImports flag. Otherwise the npm start command leads to an error (see image).
image

Note:

All other templates (React and Vue) have the allowSyntheticDefaultImports flag in their tsconfig.json.

Testing

This is already covered by current E2E tests.


CDX-280

@github-actions
Copy link
Contributor

github-actions bot commented May 2, 2021

Pull Request Report

PR Title

✅ Title follows the conventional commit spec.

Copy link
Collaborator

@louis-bompart louis-bompart left a comment

Choose a reason for hiding this comment

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

The way we modify the tsconfig.json seems odd to me.
Can't we as a schematic author act upon the tsconfig in a less roundabout way?
I was wondering especially after reading this page from Angular documentation:

Your library needs a custom Typescript configuration file with instructions on how to compile your schematics into your distributed library.

https://angular.io/guide/schematics-for-libraries#building-your-schematics

@y-lakhdar
Copy link
Contributor Author

The way we modify the tsconfig.json seems odd to me.
Can't we as a schematic author act upon the tsconfig in a less roundabout way?
I was wondering especially after reading this page from Angular documentation:

Your library needs a custom Typescript configuration file with instructions on how to compile your schematics into your distributed library.

https://angular.io/guide/schematics-for-libraries#building-your-schematics

mmh.. I think that custom tsconfig file is only used in the schematic compilation rather than being applied to the generated project.
Another option would be to have our own tsconfig file within the template. However, it means completely overriding the existing one.

@olamothe
Copy link
Member

olamothe commented May 3, 2021

Another approach might be to just skipLibCheck=true in ts config ?

If that works, I'd prefer this one. Otherwise these kind of problem will keep popping up from time to time.

@louis-bompart
Copy link
Collaborator

The way we modify the tsconfig.json seems odd to me.
Can't we as a schematic author act upon the tsconfig in a less roundabout way?
I was wondering especially after reading this page from Angular documentation:

Your library needs a custom Typescript configuration file with instructions on how to compile your schematics into your distributed library.

https://angular.io/guide/schematics-for-libraries#building-your-schematics

mmh.. I think that custom tsconfig file is only used in the schematic compilation rather than being applied to the generated project.
Another option would be to have our own tsconfig file within the template. However, it means completely overriding the existing one.

Do you know if we can detect if we're generating or adding the project?
What I would do iff we're detecting that we're adding is not modify the tsconfig but indicate that our template either requires:

  • skipLibCheck as @olamothe proposed
  • allowSyntheticDefaultImports, as currently proposed.

I think we should also try to see what would be the impact of both on the customer project, what does he win or lose.
We should also check what are the good practices of the Angular schematics in general.

Tho, if the issue is pressing, we can also proceed with a fix-it twice logic: Fix it with the proposed changes here (it works fine really), and investigate afterwards to see what's the 'best' fix.

(Personnaly, I locked the headless version to 0.10.0 for my test)

@y-lakhdar
Copy link
Contributor Author

The way we modify the tsconfig.json seems odd to me.
Can't we as a schematic author act upon the tsconfig in a less roundabout way?
I was wondering especially after reading this page from Angular documentation:

Your library needs a custom Typescript configuration file with instructions on how to compile your schematics into your distributed library.

https://angular.io/guide/schematics-for-libraries#building-your-schematics

mmh.. I think that custom tsconfig file is only used in the schematic compilation rather than being applied to the generated project.
Another option would be to have our own tsconfig file within the template. However, it means completely overriding the existing one.

Do you know if we can detect if we're generating or adding the project?
What I would do iff we're detecting that we're adding is not modify the tsconfig but indicate that our template either requires:

  • skipLibCheck as @olamothe proposed
  • allowSyntheticDefaultImports, as currently proposed.

I think we should also try to see what would be the impact of both on the customer project, what does he win or lose.
We should also check what are the good practices of the Angular schematics in general.

Tho, if the issue is pressing, we can also proceed with a fix-it twice logic: Fix it with the proposed changes here (it works fine really), and investigate afterwards to see what's the 'best' fix.

(Personnaly, I locked the headless version to 0.10.0 for my test)

Yes, we can detect if we are adding or updating a file. However in this case, I'm pretty sure we will always be updating the file.
Angular schematic SDK offers plenty of util functions to dynamically insert code fragment into the project tree, but nothing specific to the tsconfig file.
We can go with a stronger approach and use a public method such as insertAfterLastOccurrence and prevent a hackish JSON parse.

@olamothe, skipLibCheck=true works

@y-lakhdar y-lakhdar merged commit f633459 into master May 3, 2021
@louis-bompart louis-bompart deleted the CDX-280 branch July 29, 2021 16:48
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.

3 participants