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(parcel): more robust handling of missing package.json #882

Merged
merged 6 commits into from
May 14, 2019

Conversation

trieloff
Copy link
Contributor

fixes #881

@bdelacretaz does this fix your issue?

@codecov
Copy link

codecov bot commented May 14, 2019

Codecov Report

Merging #882 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #882      +/-   ##
==========================================
+ Coverage   91.76%   91.77%   +<.01%     
==========================================
  Files          43       43              
  Lines        1786     1787       +1     
==========================================
+ Hits         1639     1640       +1     
  Misses        147      147
Impacted Files Coverage Δ
src/parcel/AdapterJSAsset.js 93.75% <100%> (+0.2%) ⬆️

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 0855c48...8c3fe34. Read the comment docs.

@bdelacretaz
Copy link

It does fix it but it's not setting pkg.devDependencies anymore, dunno if that's required.

I was just submitting my own fix, #883, I'll let you select the right one!

@trieloff
Copy link
Contributor Author

I just noticed. Both a missing package.json and missing devDependencies would be an issue.

@bdelacretaz
Copy link

I think you need

const pkg = await super.getPackage() || {}

or something like that in the current fix, as pkg itself might be null. It's still failing for me with that fix.

My fix at https://github.com/adobe/helix-cli/pull/883/files works (but maybe it's not the preferred style)

const pkg = await super.getPackage() || {};
const pack = pkg.devDependencies ? pkg : Object.assign(pkg, {
devDependencies: {
hyperapp: '*',
Copy link
Contributor

Choose a reason for hiding this comment

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

this is set twice now...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only when devDependencies didn't exist before.

Copy link
Contributor

Choose a reason for hiding this comment

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

why do you need it, btw ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In order to get parcel to think that the pragma for JSX files is h instead of React.createElement without forcing people to add it to their JSX files.

@trieloff trieloff merged commit b423149 into master May 14, 2019
@adobe-bot
Copy link
Collaborator

🎉 This PR is included in version 2.5.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

@kptdobe
Copy link
Contributor

kptdobe commented May 14, 2019

I do not see a newly added test that would prevent for this bug to happen again...

@tripodsan tripodsan deleted the fix-missing-package branch May 15, 2019 00:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

hlx up: cannot read property 'devDependencies' of undefined
5 participants