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

[wip] support aliases #237

Conversation

ChristopherBiscardi
Copy link
Contributor

@ChristopherBiscardi ChristopherBiscardi commented Mar 5, 2020

This adds support for webDependencies alias support. For example, we can alias react to preact/compat.

{
 "snowpack": {
    "webDependencies": [
      "preact",
      "@emotion/preact-core",
      [
        "react",
        "preact/compat"
      ]
    ]
  }
}

I still need to get some local tests passing and go back over the code to make sure it's doing what I think it is.

TODO:

  • fix: source-pika tests (node 13 only?)
  • fix: export tests (node 13 only?) [fixed by rebasing]
  • write: alias tests

@FredKSchott
Copy link
Owner

This looks great, thanks Chris! Will do a deeper review of the code asap.

Looks like master branch got bit by a change to Node.js v13 in todays release. I just pushed a fix to master, so now master should be green.

@ChristopherBiscardi
Copy link
Contributor Author

current problems: It seems like rollup needs some way to track import React from 'react' back to preact/compat entrypoint. I could write a babel plugin that swaps imports so you'd end up with import React from 'preact/compat' but I don't know enough about rollup to know if that would fix it.

For reference, after installing react-helmet with the webDependencies: [['react', 'preact/compat']] config, I'm working on fixing this error:

➜ yarn snowpack
yarn run v1.19.0
$ snowpack --dest public/web_modules
⠼ snowpack installing... @emotion/preact-core, preact, preact/compat, react-helmet
✖ 'react' is imported by '../../node_modules/react-helmet/lib/Helmet.js', but could not be resolved.
  Make sure that the package is installed and that the file exists.
✖ 'react' is imported by '../../node_modules/react-helmet/lib/HelmetUtils.js', but could not be resolved.
  Make sure that the package is installed and that the file exists.
✖ 'react' is imported by '../../node_modules/react-side-effect/lib/index.js', but could not be resolved.
  Make sure that the package is installed and that the file exists.
✖ 'react' is imported by 'react?commonjs-external', but could not be resolved.
  Make sure that the package is installed and that the file exists.
✔ snowpack installed: @emotion/preact-core, preact, preact/compat, react-helmet. [1.05s]
⚠ Finished with warnings.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

@FredKSchott
Copy link
Owner

Take a look at https://github.com/rollup/plugins/tree/master/packages/alias. At best, you can leverage this Rollup plugin directly, but at worse there might still be something useful in the codebase to borrow from.

@ChristopherBiscardi
Copy link
Contributor Author

alias plugin seems to be working locally. Just have to do some tests+fixes now.

Copy link
Owner

@FredKSchott FredKSchott left a comment

Choose a reason for hiding this comment

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

This lgtm, once you figure out broken tests.

The interface may need to change a bit after this, if you don't mind carrying that conversation forward into #236. But overall very +1 on supporting this in some way into the future.

src/index.ts Outdated
@@ -223,6 +224,7 @@ export async function install(
{hasBrowserlistConfig, isExplicit, lockfile}: InstallOptions,
config: SnowpackConfig,
) {
// throw new Error('typescript sucks'); // @ts-ignore
Copy link
Owner

Choose a reason for hiding this comment

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

lol assuming we can get rid of this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lol whooooops, thanks for the catch

@FredKSchott
Copy link
Owner

FredKSchott commented Mar 6, 2020

For the broken tests, have you tried rebasing onto master yet?
Update: uh oh, don't spend too much time on this, looks like there was a regression in Node 13.10 affecting got sindresorhus/got#1107

@ChristopherBiscardi
Copy link
Contributor Author

Rebasing fixed the original export test failure. I think the source --pika stuff is on me

@ChristopherBiscardi
Copy link
Contributor Author

left a comment on #236 about maybe moving to an object and a separate aliases key now that I know more about how this part of snowpack works. I think this allows us to mess with less of the codebase and keep aliases separate from webDependencies, as well as remote sourcing and such. Aliasing would become "just aliasing" instead of playing a sort of double duty with installing/locking down dependencies.

@FredKSchott
Copy link
Owner

Feel free to close, if #249 is the new way forward

@FredKSchott
Copy link
Owner

err, closing because I think I know the answer. but feel free to comment to reopen

@FredKSchott FredKSchott closed this Apr 2, 2020
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