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

Use updated dist glob pattern in package.json #731

Merged
merged 1 commit into from
Apr 10, 2023

Conversation

riywo
Copy link
Contributor

@riywo riywo commented Apr 4, 2023

Description of changes:

dist-* doesn't work with npm@9 and nothing is packed.

This commit fixes files to a proper glob pattern so that npm pack
can include all files with npm@9.

Before

vscode ➜ .../smithyprojections/smithy/source/typescript-codegen (main) $ npm pack
npm notice 
npm notice 📦  smithy@0.0.0
npm notice === Tarball Contents === 
npm notice 11.4kB LICENSE     
npm notice 3.1kB  package.json
...
npm notice total files:   2                                       

After

vscode ➜ .../smithyprojections/smithy/source/typescript-codegen (main) $ npm pack
npm notice 
npm notice 📦  smithy@0.0.0
npm notice === Tarball Contents === 
npm notice 11.4kB LICENSE                                      
npm notice 172B   dist-cjs/commands/index.js                   
npm notice 1.6kB  dist-cjs/commands/SayHelloCommand.js         
npm notice 601B   dist-cjs/index.js                            
npm notice 165B   dist-cjs/models/index.js                     
npm notice 1.3kB  dist-cjs/models/models_0.js                  
npm notice 436B   dist-cjs/models/SampleServiceException.js    
npm notice 6.2kB  dist-cjs/protocols/Aws_restJson1.js          
npm notice 2.3kB  dist-cjs/runtimeConfig.browser.js            
npm notice 2.8kB  dist-cjs/runtimeConfig.js                    
npm notice 558B   dist-cjs/runtimeConfig.native.js             
npm notice 891B   dist-cjs/runtimeConfig.shared.js             
npm notice 834B   dist-cjs/Sample.js                           
npm notice 2.0kB  dist-cjs/SampleClient.js                     
npm notice 35B    dist-es/commands/index.js                    
npm notice 1.5kB  dist-es/commands/SayHelloCommand.js          
npm notice 186B   dist-es/index.js                             
npm notice 28B    dist-es/models/index.js                      
npm notice 717B   dist-es/models/models_0.js                   
npm notice 277B   dist-es/models/SampleServiceException.js     
npm notice 5.8kB  dist-es/protocols/Aws_restJson1.js           
npm notice 2.0kB  dist-es/runtimeConfig.browser.js             
npm notice 2.4kB  dist-es/runtimeConfig.js                     
npm notice 387B   dist-es/runtimeConfig.native.js              
npm notice 661B   dist-es/runtimeConfig.shared.js              
npm notice 676B   dist-es/Sample.js                            
npm notice 1.6kB  dist-es/SampleClient.js                      
npm notice 35B    dist-types/commands/index.d.ts               
npm notice 1.0kB  dist-types/commands/SayHelloCommand.d.ts     
npm notice 186B   dist-types/index.d.ts                        
npm notice 28B    dist-types/models/index.d.ts                 
npm notice 1.7kB  dist-types/models/models_0.d.ts              
npm notice 376B   dist-types/models/SampleServiceException.d.ts
npm notice 548B   dist-types/protocols/Aws_restJson1.d.ts      
npm notice 1.8kB  dist-types/runtimeConfig.browser.d.ts        
npm notice 1.8kB  dist-types/runtimeConfig.d.ts                
npm notice 1.8kB  dist-types/runtimeConfig.native.d.ts         
npm notice 510B   dist-types/runtimeConfig.shared.d.ts         
npm notice 608B   dist-types/Sample.d.ts                       
npm notice 5.7kB  dist-types/SampleClient.d.ts                 
npm notice 3.1kB  package.json                                 
...
npm notice total files:   41                                      

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@riywo riywo requested review from a team as code owners April 4, 2023 05:41
@riywo
Copy link
Contributor Author

riywo commented Apr 5, 2023

Can anyone review? This is a bug blocking to use this library.

@trivikr
Copy link
Contributor

trivikr commented Apr 5, 2023

@riywo Is there a bug report on npm CLI, or Release Notes?
If not, can you create one, and reference this issue?

From their documentation, this appears to be a bug

The optional files field is an array of file patterns that describes the entries to be included when your package is installed as a dependency. File patterns follow a similar syntax to .gitignore, but reversed: including a file, directory, or glob pattern (*, **/*, and such) will make it so that file is included in the tarball when it's packed.

@riywo
Copy link
Contributor Author

riywo commented Apr 5, 2023

Good point. Will check them later.

@riywo
Copy link
Contributor Author

riywo commented Apr 6, 2023

I think the behavior has changed since npm@9. dist-** (or actually dist-*) worked with npm@8 because it's parsed as wildcard.

I cut a bug ticket but I'm not sure this is a bug: npm/cli#6330

I believe changing the directory structure to dist/** would be straightforward. If you agree, I can modify this PR with dist/** by updating tsconfigs as well.

@riywo
Copy link
Contributor Author

riywo commented Apr 10, 2023

As I thought, this is an expected change. See the closed issue.

Which way do you want to go with? Specifying all directories explicitly (current PR) or using dist/**?

@trivikr
Copy link
Contributor

trivikr commented Apr 10, 2023

Specifying all directories explicitly (current PR) or using dist/**?

Please use dist-*/**

`dist-*` doesn't work with `npm@9` and nothing is packed.

This commit fixes `files` to a proper glob pattern so that `npm pack`
can include all files with `npm@9`.
@riywo
Copy link
Contributor Author

riywo commented Apr 10, 2023

@trivikr Updated with dist-*/**.

@trivikr trivikr changed the title fix: include dist directories to package Use updated dist glob pattern in package.json Apr 10, 2023
@srchase srchase merged commit bbd7ecc into smithy-lang:main Apr 10, 2023
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