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: export REQUEST, RESPONSE tokens #688

Closed
wants to merge 1 commit into from

Conversation

AliYusuf95
Copy link

@AliYusuf95 AliYusuf95 commented Sep 30, 2021

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[x] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Other... Please describe:

What is the current behavior?

Issue Number: #573

What is the new behavior?

Tokens can be imported directly from @nestjs/ng-universal

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

@tincho-despegar
Copy link

Hi! Any news about this PR? I really need this to be in a release. Can I be of any help?

Copy link
Member

@micalevisk micalevisk left a comment

Choose a reason for hiding this comment

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

since the README saids we must import them from @nestjs/ng-universal/tokens:

image

you shouldn't add that export to lib/index.ts. Your commit actually reverts 508bba7 but I guess something was missed in that one because currently npm run build will leave us with the following structure:

.
├── CONTRIBUTING.md
├── dist
│   ├── angular-universal.constants.d.ts
│   ├── angular-universal.constants.js
│   ├── angular-universal.module.d.ts
│   ├── angular-universal.module.js
│   ├── angular-universal.providers.d.ts
│   ├── angular-universal.providers.js
│   ├── cache
│   ├── index.d.ts
│   ├── index.js
│   ├── interfaces
│   ├── lib
│   ├── tokens.d.ts
│   ├── tokens.js
│   └── utils
├── index.d.ts
├── index.js
├── index.ts
├── jest.config.js
├── lib
│   ├── angular-universal.constants.ts
│   ├── angular-universal.module.ts
│   ├── angular-universal.providers.ts
│   ├── cache
│   ├── index.ts
│   ├── interfaces
│   ├── tokens.ts
│   └── utils
├── LICENSE
├── package.json
├── package-lock.json
├── README.md
├── renovate.json
├── schematics
│   ├── collection.json
│   └── install
├── tsconfig.json
├── tsconfig.schematics.json
└── tslint.json

so there's no tokens.d.ts nor tokens/index.d.ts (at the root) to resolve to

@AliYusuf95
Copy link
Author

AliYusuf95 commented Dec 18, 2021

@micalevisk thank you, I didn't notice that.
I created new tokens.ts on the root to solve the issue. Also, I reverted changes in lib/index.ts.

As you said the current build only build ./lib directory to ./dist while the root index.ts only export whatever in ./dist/index, I did the same for tokens.

@AliYusuf95
Copy link
Author

@kamilmysliwiec Since the current index.ts in the root it's not affected by the build command due to the wrong path in tsconfig.json. I suggest having a separate command to build the root index and tokens.

@bastienlemaitre
Copy link

Up

@DaSchTour
Copy link

😩

@DaSchTour
Copy link

@kamilmysliwiec would you please consider merging this PR before releasing a new version?

@DaSchTour
Copy link

DaSchTour commented Sep 16, 2022

As a workaround I just published a package with this fix. You can drop it into the current installation without changing imports by adding "@nestjs/ng-universal": "npm:@nestjs-fixed/[email protected]", to your package.json.

BTW: @AliYusuf95 the path must be ./dist/tokens to work properly.

@kamilmysliwiec
Copy link
Member

#688 (review)

@nestjs nestjs locked and limited conversation to collaborators Feb 9, 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.

6 participants