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

[esinstall] enable native esm for node v12+ #1093

Merged
merged 5 commits into from
Sep 21, 2020
Merged

[esinstall] enable native esm for node v12+ #1093

merged 5 commits into from
Sep 21, 2020

Conversation

FredKSchott
Copy link
Owner

@FredKSchott FredKSchott commented Sep 19, 2020

/cc @ChristopherBiscardi

Changes

Testing

  • Covered by tests

@FredKSchott FredKSchott requested a review from a team as a code owner September 19, 2020 04:54
@vercel
Copy link

vercel bot commented Sep 19, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/pikapkg/snowpack/7ibq5lqzv
✅ Preview: https://snowpack-git-mjs.pikapkg.vercel.app

@FredKSchott FredKSchott changed the title Mjs enable native mjs for node v14 Sep 19, 2020
@FredKSchott FredKSchott changed the title enable native mjs for node v14 enable native mjs for node v12+ Sep 19, 2020
@FredKSchott FredKSchott changed the title enable native mjs for node v12+ [esinstall] enable native mjs for node v12+ Sep 19, 2020
@FredKSchott FredKSchott changed the title [esinstall] enable native mjs for node v12+ [esinstall] enable native esm for node v12+ Sep 19, 2020
@FredKSchott
Copy link
Owner Author

I went one step further and removed @pika/pack from Snowpack as well. I love @pika/pack as much as the next person, but in this exact usecase it was causing more harm than good. See the contributing docs for all the weirdness that this removes.

Also, we now have a build:watch command: make changes and see the reflected instantly. Way overdue, imo!

yarn --force # only needed after very first build; afterward can be skipped
```

#### Why is `yarn --force` needed?
Copy link
Contributor

Choose a reason for hiding this comment

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

😍 😍 😍

Copy link
Contributor

@gr2m gr2m left a comment

Choose a reason for hiding this comment

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

I have tested the pull request locally with the example documented in https://github.com/pikapkg/snowpack/tree/0fe3dccb4b957d301718cfac82abbb035e740d17/esinstall#esinstall and worked as expected 👍🏼

@FredKSchott FredKSchott merged commit 20388e4 into master Sep 21, 2020
@FredKSchott FredKSchott deleted the mjs branch September 21, 2020 00:37
@drwpow
Copy link
Collaborator

drwpow commented Sep 26, 2020

I’m seeing some weird behavior in the tests, I think from this PR?

Before, all of our test repos symlinked node_modules/.bin/snowpack to the snowpack bin in pkg/. That means we were testing locally.

Now, it seems all of our build tests download snowpack from npm, which means we’re no longer testing the local version (so any test success or failure in a PR can’t be trusted).

To confirm this, in snowpack/package.json I see version 2.12.0-pre.1. But inside test/**/node_modules/snowpack/package.json, I see a separate copy with 2.11.1 (latest on npm). Are we sure our tests are still using the local build?

@FredKSchott
Copy link
Owner Author

Hmm, I do see "snowpack" being installed locally, but the bin is still from the symlinked. Something is definitely going on here though, let me update the yarn.lock file and see if that helps.

@FredKSchott
Copy link
Owner Author

Okay, think that fixed it. I think it was the 2.12.0-pre.1 release, which yarn seemed to not like when it came time to resolve workspace deps

@drwpow
Copy link
Collaborator

drwpow commented Sep 26, 2020

Ah good to know! Sorry—my mistake. 🤔 What if in the test deps we just did "snowpack": "file:../../…"? Would that be a little less “magic”?

@FredKSchott
Copy link
Owner Author

+1 if that means also moving the tests out of lerna/yarn workspaces entirely :) (at that point we’d only be using them to manage the test deps, which we’ve been bitten by “did this snapshot change because of a test dep or a Snowpack dep” when doing a fresh “yarn install”)

@drwpow
Copy link
Collaborator

drwpow commented Sep 26, 2020

As long as tests surface regressions in a PR before merging that’s all that matters to me! I don’t care if they’re a part of the Lerna/Yarn workspace or not. As long as CI runs quickly and setup for contributors is easy, that’s mostly what matters.

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.

4 participants