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

Change package.json#main to .cjs file #228

Merged
merged 5 commits into from
Jun 25, 2020
Merged

Change package.json#main to .cjs file #228

merged 5 commits into from
Jun 25, 2020

Conversation

Andarist
Copy link
Collaborator

It seems that node@12.16.3 ships with unflagged module support BUT without support for conditional exports, so it just tries to use "main" but at the same type it sees "type": "module" and fails on requireing Stylis because it thinks it's going to load a module and it can't do that when using require.

This node version is used by Vercel and AWS, so it's pretty important for us to support them. The issue can be reproduced with this version of node using this repository: https://github.com/harrisrobin/vercel-deploy-issue

@coveralls
Copy link

coveralls commented Jun 25, 2020

Pull Request Test Coverage Report for Build 28489f544603358a5ed1bfb77bba15d1aeefd089-PR-228

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

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 5c77aea8e25cc43389c235569b86ad5a1509240e: 0.0%
Covered Lines: 236
Relevant Lines: 236

💛 - Coveralls

@thysultan
Copy link
Owner

We could also change the umd file to just stylis.cjs and remove the cjs file since it can serve both.

@Andarist
Copy link
Collaborator Author

@thysultan sure, that works too - I've pushed out the change. Please remember about clearing dist before releasing a new version so there is no leftover UMD file from previous builds.

@thysultan
Copy link
Owner

Please remember about clearing dist before releasing a new version

Can you add that to the build process? i.e before generating the dist files. Does rollup provide an opt for this?

@Andarist
Copy link
Collaborator Author

Rollup itself doesn't have an option for this. I usually just use rimraf (which works on all platforms, including Windows) for this and thus I've added it here.

@thysultan
Copy link
Owner

When does prebuild run? or is that a rollup thing?

@Andarist
Copy link
Collaborator Author

Its npm scripts, you can add pre and post scripts for any other script. It works with all packaga managers

@thysultan
Copy link
Owner

We can change the "start" script to "npm run build -- --watch" so it can benefit from this as well.

@thysultan
Copy link
Owner

Other than that LGTM.

@Andarist
Copy link
Collaborator Author

Done.

@thysultan thysultan self-requested a review June 25, 2020 17:04
@thysultan thysultan merged commit 3a8972a into master Jun 25, 2020
@thysultan thysultan deleted the Andarist-patch-1 branch June 25, 2020 21:21
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.

None yet

3 participants