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

Modernize package #22

Merged
merged 13 commits into from
Mar 6, 2021
Merged

Modernize package #22

merged 13 commits into from
Mar 6, 2021

Conversation

copleykj
Copy link
Member

Setup typescript, linting, and commit hooks. Also made the package server only to avoid issues when used client side.

@copleykj copleykj requested a review from coagmano March 12, 2020 15:42
Copy link
Member

@coagmano coagmano left a comment

Choose a reason for hiding this comment

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

Looks good, just needs some explainer for client-only modules. I'll try to come up with some text later today

CHANGELOG.md Outdated

### Breaking Changes

- Made package server only. There isn't any reason for code to run on the client and doing so has been a confusing experience in the past. This will break some current usage if code is used on the client.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe a mention that this update will cause false-negatives for all modules that are only imported in client code, and that in 1.0.0+ calls to checkNpmVersions must be followed by a require for the desired package

@@ -15,6 +17,8 @@ checkNpmVersions({
const Griddle = require('griddle-react');
```

>This must be run on the server so that it has access to the package.json of the package where it can examine the version that is installed.

Copy link
Member

Choose a reason for hiding this comment

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

Add section for client-only package instructions, since you will need to statically import either the whole package, or just the package.json for the module to be included in the server bundle so we can check that it exists.

checkNpmVersions({
  'client-only': '1.3.x'
}, 'my:package');
require('client-only/package.json');

Copy link
Member Author

Choose a reason for hiding this comment

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

What about adding a check to make sure we are in development and effectively turn the checkNpmVersions into a noop for production?

@copleykj
Copy link
Member Author

copleykj commented May 8, 2020

Ok, so I've finally gotten around and tested things out quite a bit and it's looking like that because of the way the build system works, the server does not have access to the package.json of a package unless it's imported on the server. This means that leaving the package so that it runs in both environments is probably best for usability. Unfortunately this won't give any savings to the bundle size, but I think that maybe if package authors use dynamic imports this could be accomplished on their end.

@copleykj copleykj merged commit 9e02379 into master Mar 6, 2021
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.

2 participants