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

make it harder for the optimizer to incorrectly export DEV twice #1664

Merged
merged 1 commit into from
Apr 2, 2023

Conversation

xunilrj
Copy link
Contributor

@xunilrj xunilrj commented Mar 31, 2023

Summary

Try to fix #1660. I think skypack is complaining because dev.cjs after being optimized has

exports.DEV = void 0;
exports.DEV = {
  hooks: DevHooks,
  writeSignal,
  registerGraph
};

This change make it harder for the optimizer to export DEV twice.

const DEV = "_SOLID_DEV_"
  ? ({ hooks: DevHooks, writeSignal, registerGraph } as {
      readonly hooks: typeof DevHooks;
      readonly writeSignal: typeof writeSignal;
      readonly registerGraph: typeof registerGraph;
    })
  : undefined;
export { DEV };

How did you test this change?

I can´t test if this will fix skypack build.

@changeset-bot
Copy link

changeset-bot bot commented Mar 31, 2023

⚠️ No Changeset found

Latest commit: 705820c

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@thetarnav
Copy link
Contributor

Why not use a ternary? ("SOLID_DEV" ? { ... } : undefined)
The DEV object is used in libraries as a development mode check, so it being falsy needs to cause that code path to be tree-shaken.
Also the same DEV object is exported from the /store module.

@xunilrj
Copy link
Contributor Author

xunilrj commented Mar 31, 2023

Good idea. The optimizer seems happy with that.

const DEV = {
  hooks: DevHooks,
  writeSignal,
  registerGraph
} ;
exports.DEV = DEV;

@coveralls
Copy link

coveralls commented Apr 2, 2023

Pull Request Test Coverage Report for Build 4581620121

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 3 of 5 (60.0%) changed or added relevant lines in 1 file are covered.
  • 3 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.1%) to 93.963%

Changes Missing Coverage Covered Lines Changed/Added Lines %
packages/solid/src/index.ts 3 5 60.0%
Files with Coverage Reduction New Missed Lines %
packages/solid/src/index.ts 3 93.81%
Totals Coverage Status
Change from base Build 4561177901: -0.1%
Covered Lines: 4080
Relevant Lines: 4277

💛 - Coveralls

@ryansolid ryansolid merged commit 448f7c8 into solidjs:main Apr 2, 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.

v1.7 does not build on skypack
4 participants