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

feat(backend): set nestjs target to es2017 and add incremental flag #1770

Closed
wants to merge 1 commit into from
Closed

feat(backend): set nestjs target to es2017 and add incremental flag #1770

wants to merge 1 commit into from

Conversation

mehrad-rafigh
Copy link
Contributor

@mehrad-rafigh mehrad-rafigh commented Aug 30, 2019

Current Behavior (This is the behavior we have today, before the PR is merged)

target is set to es2015

Expected Behavior (This is the new behavior we can expect after the PR is merged)

target is set to es2017 and incremental flag has been set to true

Issue

Closes #1002

@mehrad-rafigh mehrad-rafigh changed the title Feature/set nestjs target to es6 Feature/set nestjs target to es2017 Aug 30, 2019
@mehrad-rafigh mehrad-rafigh changed the title Feature/set nestjs target to es2017 feature/set-nestjs-target-to-es2017 Aug 30, 2019
@mehrad-rafigh mehrad-rafigh changed the title feature/set-nestjs-target-to-es2017 feat(nx): set-nestjs-target-to-es2017 Aug 30, 2019
@mehrad-rafigh mehrad-rafigh changed the title feat(nx): set-nestjs-target-to-es2017 feat(nx): set nestjs target to es2017 and add incremental flag Aug 30, 2019
@mehrad-rafigh mehrad-rafigh changed the title feat(nx): set nestjs target to es2017 and add incremental flag feat(backend): set nestjs target to es2017 and add incremental flag Sep 1, 2019
@vsavkin
Copy link
Member

vsavkin commented Sep 2, 2019

The PR looks good. One thing that is left is adding a migration that will update the version of nest in existing projects. This is an example:

https://github.com/nrwl/nx/blob/master/packages/workspace/src/migrations/update-8-3-0/update-ng-cli-8-1.ts

@vsavkin vsavkin self-assigned this Sep 2, 2019
@mehrad-rafigh
Copy link
Contributor Author

Hi @vsavkin. Thank you for reviewing the PR. I will update this PR asap

@mehrad-rafigh
Copy link
Contributor Author

Hi @vsavkin. I updated my PR. It includes a migration and I added a test for it as well.
Should we also update the corresponding tsconfig.json files to include the target and incremental flag?

…flag

build(nx): update nestjs to latest version
@vsavkin
Copy link
Member

vsavkin commented Sep 23, 2019

@mehrad-rafigh

Quick question. Since we are bunlding nestjs apps with webpack, how does "incremental" work together with webpack? Cause all the compilation happens in memory.

@mehrad-rafigh
Copy link
Contributor Author

Hi @vsavkin.
I took a closer look into ts-loader and it seems, the incremental flag would be ignored by the ts-loader. The incremental flag has not been implemented by the ts-loader and there is an open issue

I would suggest to remove the incremental flag for the time being and open a PR, once the issue has been resolved.

What do you think?

@vsavkin
Copy link
Member

vsavkin commented Oct 4, 2019

@mehrad-rafigh it sounds good. let's wait for it to land and then open the PR.

@vsavkin
Copy link
Member

vsavkin commented Oct 15, 2019

I'm sorry but I'm going to close this. We changed the way we are handling package updates to use updatePackagesInPackageJson.

@vsavkin vsavkin closed this Oct 15, 2019
@mehrad-rafigh mehrad-rafigh deleted the feature/set-nestjs-target-to-es6 branch October 21, 2019 07:46
@github-actions
Copy link

This pull request has already been merged/closed. If you experience issues related to these changes, please open a new issue referencing this pull request.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NestJs need to have es6 in tsconfig target
2 participants