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: OutputBundle Tagged union with 'type = chunk|asset' #3080

Conversation

askbeka
Copy link
Contributor

@askbeka askbeka commented Aug 24, 2019

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

List any relevant issue numbers:
#3079

Description

I want to make it easier to develop plugins using typescript.
It is possible since Rollup is implemented in typescript. But there are few problems.

One of which is one fixed here, and reported in the issues mentioned above

@shellscape
Copy link
Contributor

@askbeka thank you for submitting a pull request!

Thanks as well for providing the related issue number. Please do fill in the description in the PR template as well, and please be verbose. That'll really help out the folks reviewing it.

src/rollup/types.d.ts Outdated Show resolved Hide resolved
@askbeka askbeka force-pushed the feat/types-bundle-tagged-union-type-asset-chunk branch from 4e74c4c to 7b47ad0 Compare August 24, 2019 15:13
@lukastaegert
Copy link
Member

Personally I like going for type, especially since it would actually be possible to do a soft-deprecation by making isAsset a getter here:

output.bundle[fileName] = { fileName, isAsset: true, source };

Then when isAsset is accessed, this getter should do seomthing like graph.warnDeprecation('Accessing "isAsset" on files in the bundle is deprecated, please use "type === \'asset\'" instead', false).

The false signifies that this is an upcoming deprecation and this message will only be shown if people use the strictDeprecations flag. In Rollup@2, all "inactive" deprecations will become "active" while all active ones are removed, so this flag will then be removed with Rollup@3.

@askbeka
Copy link
Contributor Author

askbeka commented Aug 25, 2019

@lukastaegert thanks. I will add this message.
Only thing is that only having getter will make it readonly property. I am ot sure if it will be breaking change. Maybe someone is modifying the property. I can check that.

In addition to that isAsset method can be added to Plugin context, to make it easier for JS users, it can frustrating to debug issues related to spelling:

isAsset(chunkOrAsset: OutputChunk | OutputAsset): chukOrAsset is OutputAsset

And one question. Why types.d.ts is a declarations file why not just .ts?
This limits typings and leads to duplication.
For example type field in interface declaration could be derived from constant

export const CHUNK_TYPE = 'chunk';

export interface OutputChunk {
  type: typeof CHUNK_TYPE;
}

Later constant could be reused.

src/rollup/types.d.ts Outdated Show resolved Hide resolved
src/rollup/types.d.ts Outdated Show resolved Hide resolved
@askbeka askbeka force-pushed the feat/types-bundle-tagged-union-type-asset-chunk branch 3 times, most recently from 6fb3c67 to ddb330c Compare August 26, 2019 23:56
@codecov
Copy link

codecov bot commented Aug 26, 2019

Codecov Report

Merging #3080 into master will increase coverage by <.01%.
The diff coverage is 87.5%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3080      +/-   ##
==========================================
+ Coverage   89.06%   89.06%   +<.01%     
==========================================
  Files         165      165              
  Lines        5713     5715       +2     
  Branches     1738     1738              
==========================================
+ Hits         5088     5090       +2     
  Misses        383      383              
  Partials      242      242
Impacted Files Coverage Δ
src/utils/FileEmitter.ts 100% <100%> (ø) ⬆️
src/rollup/index.ts 93.65% <100%> (-0.04%) ⬇️
cli/run/build.ts 80.48% <50%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 18d1aed...2f317ac. Read the comment docs.

@lukastaegert
Copy link
Member

Only thing is that only having getter will make it readonly property. I am ot sure if it will be breaking change. Maybe someone is modifying the property.

Modifying this property would break Rollup anyway so I think we should be safe here.

In addition to that isAsset method can be added to Plugin context, to make it easier for JS users, it can frustrating to debug issues related to spelling

Not sure about this. Adding new properties to the context is always a bit of maintenance overhead, needs to be documented etc. And in this case, no additional information is made available. I would prefer to avoid this in order to keep the API surface as slim as possible.

And one question. Why types.d.ts is a declarations file why not just .ts?

This is unfortunately necessary because this is how we create typings for the distributed JS files without publishing tons of unneeded types. Basically this file is copied unmodified into the dist folder when building/publishing. For that reason, this file should contain exactly the types needed to describe the public interface. I know this prevents things like what you suggested but from all alternative solutions, this is the one that provided the best output for the end user.

Copy link
Member

@lukastaegert lukastaegert left a comment

Choose a reason for hiding this comment

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

Hi, I think this is really getting into a good shape, just some final comments. Thanks!

test/form/samples/emit-asset-file/_config.js Outdated Show resolved Hide resolved
cli/run/build.ts Show resolved Hide resolved
docs/02-javascript-api.md Outdated Show resolved Hide resolved
docs/05-plugin-development.md Outdated Show resolved Hide resolved
src/rollup/index.ts Outdated Show resolved Hide resolved
src/rollup/types.d.ts Outdated Show resolved Hide resolved
src/utils/FileEmitter.ts Outdated Show resolved Hide resolved
src/rollup/types.d.ts Outdated Show resolved Hide resolved
@askbeka askbeka force-pushed the feat/types-bundle-tagged-union-type-asset-chunk branch from 68cf37c to 799667b Compare September 7, 2019 11:19
@askbeka askbeka force-pushed the feat/types-bundle-tagged-union-type-asset-chunk branch from 799667b to 38c3fed Compare September 7, 2019 11:22
@askbeka
Copy link
Contributor Author

askbeka commented Sep 7, 2019

@lukastaegert My apologies for the delay.
I have adjusted the pull request following your review.

Will try to check what is failing in codecov

@askbeka askbeka force-pushed the feat/types-bundle-tagged-union-type-asset-chunk branch from 3f5f033 to 38c3fed Compare September 7, 2019 19:54
Copy link
Member

@lukastaegert lukastaegert left a comment

Choose a reason for hiding this comment

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

Looks really good, thanks!

@lukastaegert lukastaegert merged commit bba1466 into rollup:master Sep 8, 2019
@askbeka askbeka deleted the feat/types-bundle-tagged-union-type-asset-chunk branch September 8, 2019 10:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants